bench/ddl_analysis: de-flake, clean up format, improve usability#61830
bench/ddl_analysis: de-flake, clean up format, improve usability#61830craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
This commit deflakes the expectation test by properly dealing with transaction retries and throwing away those results. Release note: None
|
This change is |
This change copies the output of the invoked benchmarks to the outer test and logs parsed results. Release note: None
e5fbe81 to
d24b8d3
Compare
rafiss
left a comment
There was a problem hiding this comment.
very exciting to have this test back!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @RichardJCai)
pkg/bench/ddl_analysis/ddl_analysis_bench.go, line 104 at r1 (raw file):
} b.ReportMetric(float64(roundTrips)/float64(b.N), "roundtrips")
i'm not super familiar with custom metrics in benchmarks but it seems like the docs say If the metric is per-iteration, the caller should divide by b.N -- so do we need to divide here? it looks like it's outside of the loop
rafiss
left a comment
There was a problem hiding this comment.
meant to lgtm, just had a question about tests
Reviewable status:
This makes it slightly easier to read and update. Release note: None
d24b8d3 to
70abbfc
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/bench/ddl_analysis/ddl_analysis_bench.go, line 104 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i'm not super familiar with custom metrics in benchmarks but it seems like the docs say
If the metric is per-iteration, the caller should divide by b.N-- so do we need to divide here? it looks like it's outside of the loop
Note that this variable is accumulated per iteration. The metric is per iteration. We are dividing here in order to fit the API.
The old code used to only look at the last iteration 😬
ajwerner
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
|
Build succeeded: |
See individual commits.
Fixes #57771.