Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

asim: add random predefined cluster config selection #107075

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

wenyihu6
Copy link
Contributor

@wenyihu6 wenyihu6 commented Jul 18, 2023

This patch takes the first step towards a randomized framework by enabling asim
testing to randomly select a cluster information configuration from a set of
predefined choices. These choices are hardcoded and represent common cluster
configurations.

TestRandomized now takes in true for randOptions.cluster to enable random
cluster config selection. This provides two modes for cluster generation:

  1. Default: currently set to 3 nodes and 1 store per node
  2. Random: randomly select a predefined cluster info

Part of: #106311

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@wenyihu6 wenyihu6 changed the title New cluster asim: introduce rand testing framework Jul 18, 2023
@wenyihu6 wenyihu6 force-pushed the new-cluster branch 4 times, most recently from a8832ac to 3a1140e Compare July 20, 2023 12:22
@wenyihu6 wenyihu6 changed the title asim: introduce rand testing framework asim: add random predefined cluster config selection Jul 20, 2023
@wenyihu6 wenyihu6 force-pushed the new-cluster branch 13 times, most recently from 3a3d69d to 3ac9cc7 Compare July 24, 2023 20:26
@wenyihu6 wenyihu6 force-pushed the new-cluster branch 10 times, most recently from 2bf7525 to 28ad48a Compare August 1, 2023 14:48
@wenyihu6 wenyihu6 marked this pull request as ready for review August 1, 2023 14:51
@wenyihu6 wenyihu6 requested a review from a team as a code owner August 1, 2023 14:51
Copy link
Collaborator

@kvoli kvoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the first 3 commits from the prior PRs? Could you add a note, or just rebase once these merge.

Only 1 comment on the last commit regarding the added test expectation.

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 6 of 6 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @wenyihu6)


pkg/kv/kvserver/asim/state/config_loader.go line 267 at r4 (raw file):

// LoadClusterInfo adds stores to State.
func (c ClusterInfo) GetNumOfStores() (totalStores int) {
	for _, r := range c.Regions {

Should we also specify storesPerNode explicitly in the ClusterInfos above?

Also could you remind me where GetNumOfStores is going to be used? Is it just in testing atm?

@wenyihu6
Copy link
Contributor Author

wenyihu6 commented Aug 1, 2023

The first three commits are from #107696.

@wenyihu6
Copy link
Contributor Author

wenyihu6 commented Aug 1, 2023

pkg/kv/kvserver/asim/state/config_loader.go line 267 at r4 (raw file):

Previously, kvoli (Austen) wrote…

Should we also specify storesPerNode explicitly in the ClusterInfos above?

Also could you remind me where GetNumOfStores is going to be used? Is it just in testing atm?

Discussed offline - removed them.

@wenyihu6 wenyihu6 requested a review from kvoli August 1, 2023 20:06
Copy link
Collaborator

@kvoli kvoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @wenyihu6)

This patch takes the first step towards a randomized framework by enabling asim
testing to randomly select a cluster information configuration from a set of
predefined choices. These choices are hardcoded and represent common cluster
configurations.

TestRandomized now takes in `true` for randOptions.cluster to enable random
cluster config selection. This provides two modes for cluster generation:
1. Default: currently set to 3 nodes and 1 store per node
2. Random: randomly select a predefined cluster info

Part of: cockroachdb#106311
Release note: None
@wenyihu6
Copy link
Contributor Author

wenyihu6 commented Aug 2, 2023

Last commit was to rebase onto master after merging the first three commits. Will merge this PR after CI goes green.

@wenyihu6
Copy link
Contributor Author

wenyihu6 commented Aug 2, 2023

TFTR!

bors r=kvoli

@craig
Copy link
Contributor

craig bot commented Aug 2, 2023

Build succeeded:

@craig craig bot merged commit 46255c8 into cockroachdb:master Aug 2, 2023
@wenyihu6 wenyihu6 deleted the new-cluster branch August 4, 2023 19:29
@wenyihu6 wenyihu6 self-assigned this Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants