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

builtins: force production values in TestSerialNormalizationWithUniqueUnorderedID #107597

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

yuzefovich
Copy link
Member

We've observed that if batch-bytes-limit value is set too low, then the "key counts" query in this test takes much longer (on my laptop it was 60s for a particular random seed vs 2.4s with production values), so this commit forces some production values.

Fixes: #106829.

Release note: None

…eUnorderedID

We've observed that if `batch-bytes-limit` value is set too low, then
the "key counts" query in this test takes much longer (on my laptop it
was 60s for a particular random seed vs 2.4s with production values), so
this commit forces some production values.

Release note: None
@yuzefovich yuzefovich added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Jul 26, 2023
@yuzefovich yuzefovich requested a review from a team as a code owner July 26, 2023 03:49
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@andyyang890 andyyang890 left a comment

Choose a reason for hiding this comment

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

Thanks for investigating and fixing this! Do you mind explaining how you determined that it was because of a low default-batch-bytes-limit value?

Copy link
Collaborator

@DrewKimball DrewKimball 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 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

@yuzefovich
Copy link
Member Author

My investigation was roughly as follows:

  • take a look at the goroutine dump to see whether we have a deadlock or something
    • we don't as goroutine 2468 (the main query execution) isn't stuck and seems to be reading data from pebble
  • run the test locally a few times, every time was fast
  • run the test locally with the seed from test.log failure, this was slow
    • this (as well as similar issues in the past) made me think that it's metamorphic randomization to blame
  • examine the randomized values
    • initialized metamorphic constant "default-batch-bytes-limit" with value 21 stood out because I know the default is 10MiB
  • adjust the test to disable randomization of this value (as well as a few others) via ForceProductionValues testing knob
    • now the test with the fixed seed is fast again, so the randomization of default-batch-bytes-limit is the culprit.

Hope this helps, let me know if more context would be useful.

TFTRs!

bors r+

@rafiss
Copy link
Collaborator

rafiss commented Jul 26, 2023

Thanks for the explanation!

I saw that default-batch-bytes-limit has a metamorphic range from 1 byte to 64Kib. Is it useful to ever test such small values? We could make it range from 1KiB to 64 KiB instead, perhaps.

@yuzefovich
Copy link
Member Author

I do think that very small values of default-batch-bytes-limit are valuable - roughly speaking, this number controls how much data can be returned by a single BatchResponse, and in many of our tests we have very little data to return, so in order to test multiple KV requests we need to have very low value.

I do see the pain of figuring out that this particular metamorphic randomized value is to blame for slowness in a random test, but so far the cases where we had to disable it explicitly are not numerous, and we've had this randomization in place for years now.

@craig
Copy link
Contributor

craig bot commented Jul 26, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql/sem/builtins: TestSerialNormalizationWithUniqueUnorderedID failed
5 participants