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

rttanalysis: don't count leasing the database desc #93153

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

andreimatei
Copy link
Contributor

@andreimatei andreimatei commented Dec 6, 2022

A bunch of rtt-analysis tests were counting a request for leasing the database descriptor. This is not interesting. This patch makes the test framework lease it first through a "USE" statement.

The number of KV requests required for leasing is currently mis-counted. We count 1, but in reality it's 4. A different patch will correct the miscounting that, at which point that would be too significant for the tests.

Release note: None
Epic: None

@andreimatei andreimatei requested review from ajwerner and a team December 6, 2022 20:13
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andreimatei andreimatei force-pushed the rtt-analysis branch 3 times, most recently from 28e426c to d858d35 Compare December 7, 2022 18:40
@andreimatei andreimatei requested a review from a team December 7, 2022 18:40
Copy link
Contributor

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

Copy link
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)

Copy link
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

bors r-

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)

@craig
Copy link
Contributor

craig bot commented Dec 14, 2022

Canceled.

andreimatei and others added 2 commits December 14, 2022 12:43
A bunch of rtt-analysis tests were counting a request for leasing the database
descriptor. This is not interesting. This patch makes the test framework lease
it first through a "USE" statement.

The number of KV requests required for leasing is currently mis-counted. We
count 1, but in reality it's 4. A different patch will correct the miscounting
that, at which point that would be too significant for the tests.

Release note: None
Epic: None
It's useful to turn it off when you want to see if you're chaning any
numbers.

Release note: None
Copy link
Contributor Author

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)

@craig craig bot merged commit a30fb14 into cockroachdb:master Dec 14, 2022
@craig
Copy link
Contributor

craig bot commented Dec 14, 2022

Build succeeded:

@andreimatei andreimatei deleted the rtt-analysis branch December 14, 2022 22:10
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