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

rttanalysisccl: adjust alter primary region bench #73858

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Dec 15, 2021

fixes #73775

This is flaky, and less is better anyway.

Release note: None

This is flaky, and less is better anyway.

Release note: None
@rafiss rafiss requested review from otan and a team December 15, 2021 19:02
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss
Copy link
Collaborator Author

rafiss commented Dec 15, 2021

bors r=RichardJCai

@craig
Copy link
Contributor

craig bot commented Dec 15, 2021

Build succeeded:

@craig craig bot merged commit ef789b1 into cockroachdb:master Dec 15, 2021
@andreimatei
Copy link
Contributor

Still flaky yo - it wants 27. Fails in 15 runs on my machine if I put enough parallelism.
See a failure here: https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_UnitTests_Test/3923023

Also, consider putting more words in the commit when bumping the expectations, if some investigation of the trace was done. If no investigation was done, that's also worth noting.

@andreimatei
Copy link
Contributor

Fails in 15 runs on my machine if I put enough parallelism.

But you gotta stress the whole package.

@andreimatei
Copy link
Contributor

I also got a failure in TestBenchmarkExpectation/DropSequence/drop_1_sequence, and I doubt it's my fault.
https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_GitHubCiOptional/3923045?

@rafiss
Copy link
Collaborator Author

rafiss commented Dec 16, 2021

I also got a failure in TestBenchmarkExpectation/DropSequence/drop_1_sequence

That seems a bit more of a sql-schema thing. (Actually, I think this issue might be too, but rttanalysisccl still points to sql-exp in CODEOWNERS.) Looks like it's tracked in #73884

@ajwerner
Copy link
Contributor

I'm thinking about giving it +/- 1 for now. It's starting to drive me nuts. We mostly care about the scaling. It's pretty unsatisfying. I did add some code to make it give me a jaeger trace in hopes of being able to spot the difference in that more clearly.

@RichardJCai
Copy link
Contributor

I'm thinking about giving it +/- 1 for now. It's starting to drive me nuts. We mostly care about the scaling. It's pretty unsatisfying. I did add some code to make it give me a jaeger trace in hopes of being able to spot the difference in that more clearly.

I like this, I think most of the unexpected failures are from +/- 1. We really don't care about those failures.

@rafiss rafiss deleted the fix-rtt-alter-primary-region branch December 16, 2021 20:23
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.

ccl/benchccl/rttanalysisccl: TestBenchmarkExpectation failed
6 participants