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

tenantcostclient: tighten query RU estimate test #106769

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Jul 13, 2023

The query RU estimate test involves two runs of test queries:

  • via EXPLAIN ANALYZE to get the RU estimate
  • "vanilla" way and getting the precise RU usage.

One of the test queries is a large INSERT, and previously we didn't delete the data after the first run. As a result, the second run would operate on 2x size of the data set. This commit fixes that oversight by including a DELETE query into the test query set which allows us to tighten the delta margin from 0.75 to 0.05 (got no failures in over 1k runs with 0.05 now).

Epic: None

Release note: None

The query RU estimate test involves two runs of test queries:
- via EXPLAIN ANALYZE to get the RU estimate
- "vanilla" way and getting the precise RU usage.

One of the test queries is a large INSERT, and previously we didn't
delete the data after the first run. As a result, the second run would
operate on 2x size of the data set. This commit fixes that oversight by
including a DELETE query into the test query set which allows us to
tighten the delta margin from 0.75 to 0.05 (got no failures in over 1k
runs with 0.05 now).

Release note: None
@yuzefovich yuzefovich requested review from DrewKimball and a team July 13, 2023 18:01
@yuzefovich yuzefovich requested a review from a team as a code owner July 13, 2023 18:01
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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: Thanks for the fix!

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

Got no failures in 1hr of stressing, should be good.

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 13, 2023

Build succeeded:

@craig craig bot merged commit b6b5d28 into cockroachdb:master Jul 13, 2023
@yuzefovich yuzefovich deleted the ru-test branch July 13, 2023 19:59
yuzefovich added a commit to yuzefovich/cockroach that referenced this pull request Sep 8, 2023
In cockroachdb#106769 we tightened the RU estimate test to use 0.05 allowance, but
we just saw a case where the difference was on the order of 0.2, so this
commit bumps the allowed delta to 0.25 (which is still much better than
0.75 before cockroachdb#106769).

Release note: None
craig bot pushed a commit that referenced this pull request Sep 8, 2023
110191: kvprober: make the planner thread-safe to fix test-only race r=nvanbenschoten a=joshimhoff

#109564

**kvprober: make the planner thread-safe to fix test-only race**

This commit makes the planner thread-safe, in order to fix a test-only data race. This commit also un-skips the test that was skipped, when this bug was discovered.

Fixes: #109564
Release note: None.

110275: tenantcostclient: relax expectation in RU estimate test r=yuzefovich a=yuzefovich

In #106769 we tightened the RU estimate test to use 0.05 allowance, but we just saw a case where the difference was on the order of 0.2, so this commit bumps the allowed delta to 0.25 (which is still much better than 0.75 before #106769).

Fixes: #109732.

Release note: None

Co-authored-by: Josh Imhoff <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
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