Skip to content

asim: better outputs for data-driven tests#108059

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

asim: better outputs for data-driven tests#108059
craig[bot] merged 2 commits intocockroachdb:masterfrom
wenyihu6:new-data-driven-2

Conversation

@wenyihu6
Copy link
Copy Markdown
Contributor

@wenyihu6 wenyihu6 commented Aug 2, 2023 •
edited
Loading

asim: sort before iterating over maps when printing

Previously, the simulator iterates over an unordered map when formatting and
printing states, stores, and ranges, resulting in non-deterministic output. This
patch addresses the issue by sorting the maps by key before printing and
formatting.

Release note: none
Epic: none

asim: better outputs for data-driven tests

Previously, the randomized testing framework's output only indicates whether
each iteration passes. This lack of of detail makes checking the randomized
framework and debugging challenging. This patch adds more info to the output,
including the selected configurations, the initial state of each simulation.

Additionally, this patch removes the verbosity flag for printing history plots
as it does not seem to have any practical use cases.

New verbosity flags for eval are now supported.

"eval"
[verbose=(<[]("result_only","test_settings","initial_state","config_gen","topology","all")>)]
- verbose(default value is OutputResultOnly): used to set flags on what to
   show in the test output messages. By default, all details are displayed
   upon assertion failure.
   - result_only: only shows whether the test passed or failed, along with
   any failure messages
   - test_settings: displays settings used for the repeated tests
   - initial_state: displays the initial state of each test iteration
   - config_gen: displays the input configurations generated for each test
   iteration
   - topology: displays the topology of cluster configurations
   - all: displays everything above

Part Of: #106311
Release Note: none

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Aug 2, 2023

This change is Reviewable

@wenyihu6 wenyihu6 force-pushed the new-data-driven-2 branch 6 times, most recently from 25834f0 to 0718940 Compare August 2, 2023 20:59
@wenyihu6 wenyihu6 changed the title asim: convert randomized testing to data-driven asim: better outputs for data-driven tests Aug 2, 2023
@wenyihu6 wenyihu6 self-assigned this Aug 2, 2023
@wenyihu6 wenyihu6 force-pushed the new-data-driven-2 branch 12 times, most recently from f5c485f to 4e8902f Compare August 3, 2023 11:21
@wenyihu6 wenyihu6 mentioned this pull request Aug 3, 2023
Merged
@wenyihu6 wenyihu6 force-pushed the new-data-driven-2 branch from c9fd0b7 to 4e8902f Compare August 4, 2023 03:25
@wenyihu6 wenyihu6 force-pushed the new-data-driven-2 branch 7 times, most recently from 6ce66c1 to 0bc7449 Compare August 17, 2023 17:28
@wenyihu6 wenyihu6 force-pushed the new-data-driven-2 branch 12 times, most recently from 234a481 to ac5e103 Compare August 22, 2023 15:30
kvoli
kvoli approved these changes Aug 24, 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.

A couple non-blocking questions.

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

pkg/kv/kvserver/asim/gen/generator.go line 121 at r2 (raw file):

func (bl BasicLoad) String() string {
	return fmt.Sprintf("basic load with rw_ratio=%0.2f, rate=%0.2f, skewed_access=%t, min_block_size=%d, max_block_size=%d, min_key=%d, max_key=%d", bl.RWRatio, bl.Rate, bl.SkewedAccess, bl.MinBlockSize, bl.MaxBlockSize, bl.MinKey, bl.MaxKey)

nit: truncate to 80 chars

pkg/kv/kvserver/asim/state/state.go line 60 at r2 (raw file):

	// PrettyPrint returns a pretty formatted string representation of the
	// state (more concise than String()).
	PrettyPrint() string

nit: perhaps CompactPrint?

pkg/kv/kvserver/asim/tests/output.go line 27 at r2 (raw file):

const (
	// OutputResultOnly only shows whether the test passed or failed, along with

Was there a compelling reason to use a bitset, instead of a struct?

pkg/kv/kvserver/asim/tests/testdata/rand/rand_ranges line 136 at r2 (raw file):

# factor of 1. Assertion failures on some samples are expected due to this
# factor. When there is only one replica, the removal target in rebalancing is a
# leaseholder, it is locked into the current replica distribution. The

Could you clarify "it is locked into the current replica distribution"?

pkg/kv/kvserver/asim/tests/testdata/rand/rand_ranges line 138 at r2 (raw file):

# leaseholder, it is locked into the current replica distribution. The
# rebalancer falls back to adding one replica, hoping the rebalancing would
# happen next time this range is checked. This can case thrashing where settling

nit: cause

@wenyihu6
Copy link
Copy Markdown
Contributor Author

wenyihu6 commented Aug 24, 2023

pkg/kv/kvserver/asim/state/state.go line 60 at r2 (raw file):

Previously, kvoli (Austen) wrote…

nit: perhaps CompactPrint?

I feel prettyPrint aligns better with our repo's naming conventions. I didn't find across any functions named compactPrint, but there were quite a few named with PrettyPrint.

@wenyihu6
Copy link
Copy Markdown
Contributor Author

wenyihu6 commented Aug 24, 2023

pkg/kv/kvserver/asim/tests/output.go line 27 at r2 (raw file):

Previously, kvoli (Austen) wrote…

Was there a compelling reason to use a bitset, instead of a struct?

If we use structs here, we would need to create a boolean field for each flag and explicitly checking each field's boolean value. I think using bitset here achieves the same goal but in a cleaner and more efficient way.

@wenyihu6 wenyihu6 force-pushed the new-data-driven-2 branch 2 times, most recently from bb8984c to 849496a Compare August 24, 2023 19:34
@wenyihu6
Copy link
Copy Markdown
Contributor Author

wenyihu6 commented Aug 24, 2023

pkg/kv/kvserver/asim/tests/testdata/rand/rand_ranges line 136 at r2 (raw file):

Previously, kvoli (Austen) wrote…

Could you clarify "it is locked into the current replica distribution"?

Done.

@wenyihu6 wenyihu6 force-pushed the new-data-driven-2 branch 3 times, most recently from 448c0f0 to 27e9bd4 Compare August 24, 2023 19:54
@wenyihu6
asim: sort before iterating over maps when printing
5f9c51f 
Previously, the simulator iterates over an unordered map when formatting and
printing states, stores, and ranges, resulting in non-deterministic output. This
patch addresses the issue by sorting the maps by key before printing and
formatting.

Release note: none
Epic: none
@wenyihu6
asim: better outputs for data-driven tests
2385787 
Previously, the randomized testing framework's output only indicates whether
each iteration passes. This lack of of detail makes checking the randomized
framework and debugging challenging. This patch adds more info to the output,
including the selected configurations, the initial state of each simulation.

Additionally, this patch removes the verbosity flag for printing history plots
as it does not seem to have any practical use cases.

New verbosity flags for eval are now supported.
```
"eval"
[verbose=(<[]("result_only","test_settings","initial_state","config_gen","topology","all")>)]
- verbose(default value is OutputResultOnly): used to set flags on what to
   show in the test output messages. By default, all details are displayed
   upon assertion failure.
   - result_only: only shows whether the test passed or failed, along with
   any failure messages
   - test_settings: displays settings used for the repeated tests
   - initial_state: displays the initial state of each test iteration
   - config_gen: displays the input configurations generated for each test
   iteration
   - topology: displays the topology of cluster configurations
   - all: displays everything above
```

Part Of: [cockroachdb#106311](https://github.com/kvoli/cockroach/issues/106311)
Release Note: none

(cherry picked from commit 27e9bd438a6ef1f6f0ed7205d91f28c9b54fd314)
@wenyihu6
Copy link
Copy Markdown
Contributor Author

wenyihu6 commented Aug 24, 2023 •
edited
Loading

After the PR approval, I did two smaller changes 1. Renamed GetCurrTime() to Curr() and GetState() to State(). (I just learned that it is recommended to avoid naming functions with get as prefix based on Golang's best practice https://google.github.io/styleguide/go/decisions#getters.) 2. Initially, we used a variable to track indexes for map iterating. Now that we use the slice for all map iteration, we can use slice's index to track positions. I made the changes to two places where map iterations happen but missed one other. I corrected that overlook.

@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 6cf3b69 into cockroachdb:master Aug 25, 2023
@wenyihu6 wenyihu6 deleted the new-data-driven-2 branch August 25, 2023 08:51
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.

3 participants

@wenyihu6 @cockroach-teamcity @kvoli