asim: add randomized range generation #107354

wenyihu6 · 2023-07-21T15:26:35Z

This patch enables random range configuration to be generated.

TestRandomized can now take another setting parameter rangeGen (default: uniform
rangeGenType, uniform keySpaceGenType, empty weightedRand).

These generators are part of the framework fields which persist across
iterations. The numbers produced by the generator shape the distribution across
iterations.

rangeKeyGenType: determines range generator type across iterations (default:
uniformGenerator, min = 1, max = 1000)
keySpaceGenType: determines key space generator type across iterations
(default: uniformGenerator, min = 1000, max = 200000)
weightedRand: if non-empty, enables weighted randomization for range
distribution

This provides three modes for range generation:

Default: currently set to uniform distribution
Random: randomly generates range distribution across stores
Weighted Randomization: enables weighted randomization for range distribution
if and only if given weightedRand is non-empty

Part of: #106311

Release note: None

cockroach-teamcity · 2023-07-21T15:26:45Z

This change is

kvoli

Nice stuff! Left some comments - lmk if you have any questions.

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

-- commits line 25 at r3:
Just a reminder to fill this out.

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

				s.SetNodeLocality(node.NodeID(), locality)
				storesRequired := z.StoresPerNode
				if storesRequired < 1 {

Could we unify this with the above addition? It feels error prone to be handling default values in two places. Also consider adding a TODO for moving these defaults to generation and panicing instead.

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

func (f randTestingFramework) getCluster() gen.ClusterGen {
	if !f.s.randOptions["cluster"] {

Convert to using struct fields, like we discussed offline.

pkg/kv/kvserver/asim/tests/rand_framework.go line 237 at r3 (raw file):

)

func convertInt64ToInt(num int64) int {

I'd prefer to just panic if here rather than truncate.

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

}

func NewRandomizedBasicRanges(

There are quite a few constructor functions but I wonder how much value they are providing? The panic below seems good - but perhaps we could scale back the encapsulation, given the structs are exported anyway?

pkg/kv/kvserver/asim/workload/workload.go line 194 at r3 (raw file):

}

func (g *uniformGenerator) Num() int64 {

From offline - it would be better to keep these as writeKey/readKey for now - then add a comment describing an intention to re-use pkg/workload/(kv|ycsb) key gens so we test with the same stuff.

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

// TestRandomized is a randomized testing framework which validates an allocator
// simulator by creating randomized configurations, exposing potential edge

nit: allocation simulation. This test is asserting on the expected behavior of allocation, not the simulator.

wenyihu6 · 2023-07-24T20:30:28Z

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

Previously, kvoli (Austen) wrote…

Could we unify this with the above addition? It feels error prone to be handling default values in two places. Also consider adding a TODO for moving these defaults to generation and panicing instead.

Do you mean something like this or this?

wenyihu6 · 2023-07-24T20:31:12Z

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

Previously, kvoli (Austen) wrote…

Convert to using struct fields, like we discussed offline.

Done.

wenyihu6 · 2023-07-24T20:46:21Z

-- commits line 25 at r3:

Previously, kvoli (Austen) wrote…

Just a reminder to fill this out.

Done.

wenyihu6 · 2023-07-24T20:47:08Z

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

Previously, kvoli (Austen) wrote…

There are quite a few constructor functions but I wonder how much value they are providing? The panic below seems good - but perhaps we could scale back the encapsulation, given the structs are exported anyway?

Agreed. Removed some constructors and added panic checks in Generate() instead.

wenyihu6 · 2023-07-24T20:54:35Z

pkg/kv/kvserver/asim/workload/workload.go line 194 at r3 (raw file):

Previously, kvoli (Austen) wrote…

From offline - it would be better to keep these as writeKey/readKey for now - then add a comment describing an intention to re-use pkg/workload/(kv|ycsb) key gens so we test with the same stuff.

This wasn't so easy since writeKey/readKey are not exported. I'm considering to mark them as exported from pkg/kv/kvserver/asim/workload/workload.go since we will want to export them when re-using key gens from pkg/workload/(kv|ycsb). Alternatively, we can re-create them here. Wdyt?

kvoli

Reviewed 10 of 10 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @wenyihu6)

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

Previously, wenyihu6 (Wenyi Hu) wrote…

Do you mean something like this or this?

I think both. It should panic, but then we also need to update the existing code.

pkg/kv/kvserver/asim/workload/workload.go line 194 at r3 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

This wasn't so easy since writeKey/readKey are not exported. I'm considering to mark them as exported from pkg/kv/kvserver/asim/workload/workload.go since we will want to export them when re-using key gens from pkg/workload/(kv|ycsb). Alternatively, we can re-create them here. Wdyt?

Let's go with the re-create here option for now.

wenyihu6 · 2023-08-01T19:55:33Z

The first four commits are from #107075.

wenyihu6 · 2023-08-01T19:57:17Z

pkg/kv/kvserver/asim/workload/workload.go line 167 at r11 (raw file):

// TODO(wenyihu6): Instead of duplicating the key generator logic in simulators,
// we should directly reuse the code from the repo pkg/workload/(kv|ycsb) to
// ensure consistent testing.

Added the comment here.

wenyihu6 · 2023-08-01T19:57:22Z

pkg/kv/kvserver/asim/workload/workload.go line 194 at r3 (raw file):

Previously, kvoli (Austen) wrote…

Let's go with the re-create here option for now.

Done.

kvoli

Just a small question re: key generator in the tests pkg. Looking great!

Reviewed 13 of 13 files at r7, 2 of 2 files at r8, 6 of 6 files at r9, 6 of 6 files at r10, 8 of 8 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @wenyihu6)

-- commits line 65 at r11:
nit: "enables random range configurations to be generated"

pkg/kv/kvserver/asim/tests/rand_framework.go line 237 at r3 (raw file):

Previously, kvoli (Austen) wrote…

I'd prefer to just panic if here rather than truncate.

Nice, thanks!

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

// newUniformKeyGen returns a generator that generates number∈[min, max] with a
// uniform distribution.
func newUniformKeyGen(min, max int64, rand *rand.Rand) generator {

What is the difference between this key gen, and the asim/workload key gen?

pkg/kv/kvserver/asim/workload/workload.go line 167 at r11 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

Added the comment here.

Ack

wenyihu6 · 2023-08-02T20:30:47Z

pkg/kv/kvserver/asim/tests/rand_framework.go line 237 at r3 (raw file):

Previously, kvoli (Austen) wrote…

Nice, thanks!

Done.

wenyihu6 · 2023-08-02T20:32:43Z

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

Previously, kvoli (Austen) wrote…

What is the difference between this key gen, and the asim/workload key gen?

I don't think so. I think we discussed in #107354 (review) that we want to re-create them. But I'm not sure if I'm interpreting the question correctly.

wenyihu6 · 2023-08-02T20:32:57Z

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

Previously, wenyihu6 (Wenyi Hu) wrote…

I don't think so. I think we discussed in #107354 (review) that we want to re-create them. But I'm not sure if I'm interpreting the question correctly.

I don't think there are any differences*

wenyihu6 · 2023-08-02T20:33:58Z

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

Previously, kvoli (Austen) wrote…

I think both. It should panic, but then we also need to update the existing code.

Done in #107075

This patch enables random range configurations to be generated. TestRandomized can now take another setting parameter rangeGen (default: uniform rangeGenType, uniform keySpaceGenType, empty weightedRand). These generators are part of the framework fields which persist across iterations. The numbers produced by the generator shape the distribution across iterations. - rangeKeyGenType: determines range generator type across iterations (default: uniformGenerator, min = 1, max = 1000) - keySpaceGenType: determines key space generator type across iterations (default: uniformGenerator, min = 1000, max = 200000) - weightedRand: if non-empty, enables weighted randomization for range distribution This provides three modes for range generation: 1. Default: currently set to uniform distribution 2. Random: randomly generates range distribution across stores 3. Weighted Randomization: enables weighted randomization for range distribution if and only if given weightedRand is non-empty Part of: cockroachdb#106311 Release note: None

kvoli

Nice, apologies for the delay!

Reviewed 5 of 8 files at r13, 4 of 4 files at r14, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @wenyihu6)

wenyihu6 · 2023-08-17T15:00:47Z

TFTR! No need to apologize : ) It was held up by the non-deterministic issue.

bors r=kvoli

craig · 2023-08-17T16:35:00Z

Build succeeded:

Bazel Essential CI (Cockroach)

wenyihu6 force-pushed the new-randranges branch 3 times, most recently from d3d5fa2 to 1dec2f8 Compare July 21, 2023 16:55

kvoli reviewed Jul 21, 2023

View reviewed changes

wenyihu6 force-pushed the new-randranges branch 11 times, most recently from d2b9399 to c46e671 Compare July 24, 2023 20:23

wenyihu6 force-pushed the new-randranges branch from c46e671 to 213e5e4 Compare July 24, 2023 20:46

wenyihu6 changed the title ~~[wip] asim: add randomized range generation~~ asim: add randomized range generation Jul 24, 2023

wenyihu6 force-pushed the new-randranges branch 3 times, most recently from e3ae6ce to 710e025 Compare July 24, 2023 21:20

kvoli reviewed Jul 25, 2023

View reviewed changes

kvoli mentioned this pull request Jul 25, 2023

asim: introduce rand testing framework with default-only settings #107255

Merged

wenyihu6 force-pushed the new-randranges branch 2 times, most recently from f187448 to 18fee8b Compare July 27, 2023 02:00

wenyihu6 force-pushed the new-randranges branch 5 times, most recently from eef4a03 to ee6b12c Compare August 1, 2023 14:49

wenyihu6 requested a review from kvoli August 1, 2023 21:48

kvoli reviewed Aug 2, 2023

View reviewed changes

wenyihu6 force-pushed the new-randranges branch 2 times, most recently from dd087d3 to f43274a Compare August 2, 2023 20:30

wenyihu6 marked this pull request as ready for review August 2, 2023 20:34

wenyihu6 requested a review from a team as a code owner August 2, 2023 20:34

wenyihu6 mentioned this pull request Aug 2, 2023

asim: convert randomized testing to data-driven #107957

Merged

wenyihu6 force-pushed the new-randranges branch from f43274a to 3f9d84e Compare August 3, 2023 00:59

wenyihu6 force-pushed the new-randranges branch from 3f9d84e to 05a7c11 Compare August 16, 2023 14:31

wenyihu6 requested a review from kvoli August 16, 2023 18:11

wenyihu6 force-pushed the new-randranges branch from 05a7c11 to 5c74f9c Compare August 16, 2023 19:50

wenyihu6 force-pushed the new-randranges branch from 5c74f9c to 5e55ef7 Compare August 16, 2023 20:34

kvoli approved these changes Aug 17, 2023

View reviewed changes

craig bot merged commit 8d5ff3f into cockroachdb:master Aug 17, 2023
2 checks passed

wenyihu6 deleted the new-randranges branch August 18, 2023 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asim: add randomized range generation #107354

asim: add randomized range generation #107354

wenyihu6 commented Jul 21, 2023 •

edited

Loading

cockroach-teamcity commented Jul 21, 2023

kvoli left a comment

wenyihu6 commented Jul 24, 2023

wenyihu6 commented Jul 24, 2023

wenyihu6 commented Jul 24, 2023

wenyihu6 commented Jul 24, 2023

wenyihu6 commented Jul 24, 2023

kvoli left a comment

wenyihu6 commented Aug 1, 2023

wenyihu6 commented Aug 1, 2023

wenyihu6 commented Aug 1, 2023

kvoli left a comment

wenyihu6 commented Aug 2, 2023

wenyihu6 commented Aug 2, 2023

wenyihu6 commented Aug 2, 2023 •

edited

Loading

wenyihu6 commented Aug 2, 2023

kvoli left a comment

wenyihu6 commented Aug 17, 2023

craig bot commented Aug 17, 2023

asim: add randomized range generation #107354

asim: add randomized range generation #107354

Conversation

wenyihu6 commented Jul 21, 2023 • edited Loading

cockroach-teamcity commented Jul 21, 2023

kvoli left a comment

Choose a reason for hiding this comment

wenyihu6 commented Jul 24, 2023

wenyihu6 commented Jul 24, 2023

wenyihu6 commented Jul 24, 2023

wenyihu6 commented Jul 24, 2023

wenyihu6 commented Jul 24, 2023

kvoli left a comment

Choose a reason for hiding this comment

wenyihu6 commented Aug 1, 2023

wenyihu6 commented Aug 1, 2023

wenyihu6 commented Aug 1, 2023

kvoli left a comment

Choose a reason for hiding this comment

wenyihu6 commented Aug 2, 2023

wenyihu6 commented Aug 2, 2023

wenyihu6 commented Aug 2, 2023 • edited Loading

wenyihu6 commented Aug 2, 2023

kvoli left a comment

Choose a reason for hiding this comment

wenyihu6 commented Aug 17, 2023

craig bot commented Aug 17, 2023

wenyihu6 commented Jul 21, 2023 •

edited

Loading

wenyihu6 commented Aug 2, 2023 •

edited

Loading