Skip to content

asim: enforce len(weighted_rand) == number of stores#108099

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
wenyihu6:new-load-clusteropt
Aug 25, 2023
Merged

asim: enforce len(weighted_rand) == number of stores#108099
craig[bot] merged 2 commits intocockroachdb:masterfrom
wenyihu6:new-load-clusteropt

Conversation

@wenyihu6
Copy link
Copy Markdown
Contributor

@wenyihu6 wenyihu6 commented Aug 3, 2023 •
edited
Loading

asim: enable option to change static option settings

This patch allows users to modify the settings for the static mode within the
randomized testing framework. The following command is now supported:

4. “change_static_option”[nodes=<int>][stores_per_node=<int>]
[rw_ratio=<float64>] [rate=<float64>] [min_block=<int>] [max_block=<int>]
[min_key=<int64>] [max_key=<int64>] [skewed_access=<bool>] [ranges=<int>]
[placement_type=<gen.PlacementType>] [key_space=<int>]
[replication_factor=<int>] [bytes=<int64>] [stat=<string>] [height=<int>]
[width=<int>]
e.g. change_static_option nodes=2 stores_per_node=3 placement_type=skewed
	- Change_static_option: modifies the settings for the static mode where no
	  randomization is involved. Note that this does not change the default
	  settings for any randomized generation.
	- nodes (default value is 3): number of nodes in the generated cluster
	- storesPerNode (default value is 1): number of store per nodes in the
	  generated cluster
	- rwRatio (default value is 0.0): read-write ratio of the generated load
	- rate (default value is 0.0): rate at which the load is generated
	- minBlock (default value is 1): min size of each load event
	- maxBlock (default value is 1): max size of each load event
	- minKey (default value is int64(1)): min key of the generated load
	- maxKey (default value is int64(200000)): max key of the generated load
	- skewedAccess (default value is false): is true, workload key generator is
	  skewed (zipf)
	- ranges (default value is 1): number of generated ranges
	- keySpace (default value is 200000): keyspace for the generated range
	- placementType (default value is gen.Even): type of distribution for how
	  ranges are distributed across stores
	- replicationFactor (default value is 3): number of replica for each range
	- bytes (default value is int64(0)): size of each range in bytes
	- stat (default value is “replicas”): specifies the output to be plotted
	  for the verbose option
	- height (default value is 15): height of the plot
	- width (default value is 80): width of the plot

In addition, verbose=(static_settings) can now be used to display settings used
for static options where no randomization is involved.

Part of: #106311
Release note: None

asim: enforce len(weighted_rand) == number of stores

Previously, we enforce that the length of a given weighted_rand cannot exceed
the number of stores. This was challenging for users as they might not know the
cluster configuration that would be generated and thus do not know the number of
stores. In addition, if the length of weighted_rand was less than total number
of stores, any stores outside of the weighted_rand range would simply have
zero replicas. This could lead to confusion.

To improve user control, this patch disables the use of weighted_rand with
randomized cluster generation. Requirements to use weighted_rand:

  1. use static option for cluster generation
  2. specify nodes(default:3) and stores_per_node(default:1) through
    change_static_option
  3. ensure len(weighted_rand) == number of stores == nodes * stores_per_node

In addition to these new rules, the following existing requirements remain in
place:

  1. weighted_rand should only be used with placement_type=weighted_rand and vice
    versa.
  2. must specify a weight between [0.0, 1.0] for each element in the array, with
    each element corresponding to a store
  3. sum of weights in the array should be equal to 1

Resolves: #106311
Release note: None

@wenyihu6 wenyihu6 changed the title New load clusteropt asim: enforce len(weighted_rand) == number of stores Aug 3, 2023
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Aug 3, 2023

This change is Reviewable

@wenyihu6 wenyihu6 force-pushed the new-load-clusteropt branch 10 times, most recently from 95a3e00 to ed4d5a7 Compare August 17, 2023 21:57
@wenyihu6 wenyihu6 marked this pull request as ready for review August 17, 2023 22:01
@wenyihu6 wenyihu6 requested a review from a team as a code owner August 17, 2023 22:01
@wenyihu6 wenyihu6 requested a review from kvoli August 17, 2023 22:01
@wenyihu6 wenyihu6 force-pushed the new-load-clusteropt branch 2 times, most recently from b350933 to 145dd50 Compare August 18, 2023 06:49
@wenyihu6
Copy link
Copy Markdown
Contributor Author

wenyihu6 commented Aug 18, 2023

Please only review the last two commits. The preceding commits are from #108059.

@wenyihu6 wenyihu6 force-pushed the new-load-clusteropt branch 12 times, most recently from e24463c to 5966fe8 Compare August 21, 2023 17:15
@wenyihu6 wenyihu6 force-pushed the new-load-clusteropt branch 2 times, most recently from e0d8032 to 2cf6178 Compare August 25, 2023 08:50
kvoli
kvoli approved these changes Aug 25, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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:

Nice! One non-blocking question.

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

pkg/kv/kvserver/asim/tests/rand_gen.go line 257 at r1 (raw file):

)

func (c clusterGenSettings) String() string {

nit: Any reason this moved? its better to avoid moving code around if unnecessary, as it pollutes the blame history.

@wenyihu6
Copy link
Copy Markdown
Contributor Author

wenyihu6 commented Aug 25, 2023 •
edited
Loading

pkg/kv/kvserver/asim/tests/rand_gen.go line 257 at r1 (raw file):

Previously, kvoli (Austen) wrote…

nit: Any reason this moved? its better to avoid moving code around if unnecessary, as it pollutes the blame history.

I think I moved it just so that it is after all the const initialization. I've moved them back.

@wenyihu6
asim: enable option to change static option settings
ba4b474 
This patch allows users to modify the settings for the static mode within the
randomized testing framework. The following command is now supported:
```
4. “change_static_option”[nodes=<int>][stores_per_node=<int>]
[rw_ratio=<float64>] [rate=<float64>] [min_block=<int>] [max_block=<int>]
[min_key=<int64>] [max_key=<int64>] [skewed_access=<bool>] [ranges=<int>]
[placement_type=<gen.PlacementType>] [key_space=<int>]
[replication_factor=<int>] [bytes=<int64>] [stat=<string>] [height=<int>]
[width=<int>]
e.g. change_static_option nodes=2 stores_per_node=3 placement_type=skewed
	- Change_static_option: modifies the settings for the static mode where no
	  randomization is involved. Note that this does not change the default
	  settings for any randomized generation.
	- nodes (default value is 3): number of nodes in the generated cluster
	- storesPerNode (default value is 1): number of store per nodes in the
	  generated cluster
	- rwRatio (default value is 0.0): read-write ratio of the generated load
	- rate (default value is 0.0): rate at which the load is generated
	- minBlock (default value is 1): min size of each load event
	- maxBlock (default value is 1): max size of each load event
	- minKey (default value is int64(1)): min key of the generated load
	- maxKey (default value is int64(200000)): max key of the generated load
	- skewedAccess (default value is false): is true, workload key generator is
	  skewed (zipf)
	- ranges (default value is 1): number of generated ranges
	- keySpace (default value is 200000): keyspace for the generated range
	- placementType (default value is gen.Even): type of distribution for how
	  ranges are distributed across stores
	- replicationFactor (default value is 3): number of replica for each range
	- bytes (default value is int64(0)): size of each range in bytes
	- stat (default value is “replicas”): specifies the output to be plotted
	  for the verbose option
	- height (default value is 15): height of the plot
	- width (default value is 80): width of the plot

In addition, verbose=(static_settings) can now be used to display settings used
for static options where no randomization is involved.
```

Part of: cockroachdb#106311
Release note: None
@wenyihu6
asim: enforce len(weighted_rand) == number of stores
667599b 
Previously, we enforce that the length of a given `weighted_rand` cannot exceed
the number of stores. This was challenging for users as they might not know the
cluster configuration that would be generated and thus do not know the number of
stores. In addition, if the length of `weighted_rand` was less than total number
of stores, any stores outside of the `weighted_rand` range would simply have
zero replicas. This could lead to confusion.

To improve user control, this patch disables the use of weighted_rand with
randomized cluster generation. Requirements to use weighted_rand:
1. use static option for cluster generation
2. specify nodes(default:3) and stores_per_node(default:1) through
change_static_option
3. ensure len(weighted_rand) == number of stores == nodes * stores_per_node

In addition to these new rules, the following existing requirements remain in
place:
1. weighted_rand should only be used with placement_type=weighted_rand and vice
versa.
2. must specify a weight between [0.0, 1.0] for each element in the array, with
each element corresponding to a store
3. sum of weights in the array should be equal to 1

Resolves: cockroachdb#106311

Release note: None
@wenyihu6 wenyihu6 force-pushed the new-load-clusteropt branch from 2cf6178 to 667599b Compare August 25, 2023 18:18
@wenyihu6
Copy link
Copy Markdown
Contributor Author

wenyihu6 commented Aug 25, 2023

TFTR!

bors r=kvoli

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 25, 2023

Build succeeded:

@craig craig bot merged commit 503f416 into cockroachdb:master Aug 25, 2023
@wenyihu6 wenyihu6 deleted the new-load-clusteropt branch August 25, 2023 20:13
@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

Reviewers

1 more reviewer

@kvoli kvoli kvoli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@wenyihu6 wenyihu6

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

asim: add randomness to range generation

3 participants

@wenyihu6 @cockroach-teamcity @kvoli