-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
roachtest: fail YCSB for debugging if below performance expectations #34808
roachtest: fail YCSB for debugging if below performance expectations #34808
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)
pkg/cmd/roachtest/ycsb.go, line 106 at r1 (raw file):
name = fmt.Sprintf("ycsb/%s/nodes=3/cpu=%d", wl, cpus) } wl, cpus := wl, cpus
Were we messing this up before with cpus
? Yikes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/cmd/roachtest/ycsb.go, line 106 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Were we messing this up before with
cpus
? Yikes.
No, it wasn't captured by the closure before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/cmd/roachtest/ycsb.go, line 106 at r1 (raw file):
Previously, ajwerner wrote…
No, it wasn't captured by the closure before.
Oh, right. 👍
This PR is a temporary measure to aid in debugging the very peculiar cockroachdb#34458. The idea is that if a run fails to meet the expected throughput (which is above but near the bad runs), we'd like the opportunity to poke around. Release note: None
5a01291
to
c4c3717
Compare
bors r+ |
34808: roachtest: fail YCSB for debugging if below performance expectations r=ajwerner a=ajwerner This PR is a temporary measure to aid in debugging the very peculiar #34458. The idea is that if a run fails to meet the expected throughput (which is above but near the bad runs), we'd like the opportunity to poke around. Release note: None Co-authored-by: Andrew Werner <[email protected]>
Build succeeded |
These performance expectations were added in cockroachdb#34808 to help debug confusing results where performance would regress dramatically. That behavior was due to branches for old release versions with lower performance being run in CI and uploading their results to the same place as new versions. The unfortunate consequence of this change is that old versions can no longer pass these tests. Now that the mystery has been solved, this PR removes those performance checks. Fixes cockroachdb#35332. Fixes cockroachdb#35331. Release note: None
35334: roachtest: remove ycsb performance expectations r=ajwerner a=ajwerner These performance expectations were added in #34808 to help debug confusing results where performance would regress dramatically. That behavior was due to branches for old release versions with lower performance being run in CI and uploading their results to the same place as new versions. The unfortunate consequence of this change is that old versions can no longer pass these tests. Now that the mystery has been solved, this PR removes those performance checks. Fixes #35332. Fixes #35331. Release note: None Co-authored-by: Andrew Werner <[email protected]>
This PR is a temporary measure to aid in debugging the very peculiar #34458.
The idea is that if a run fails to meet the expected throughput (which is above
but near the bad runs), we'd like the opportunity to poke around.
Release note: None