storage: include version in benchmark fixture directory #97134
Conversation
|
This change is |
RaduBerinde
force-pushed
the
refresh-range-potential-fix
branch
from
5b5d1c3
to
03829c4
Compare
RaduBerinde
force-pushed
the
refresh-range-potential-fix
branch
from
03829c4
to
1ea9b89
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @bananabrick and @RaduBerinde)
pkg/kv/kvserver/batcheval/cmd_refresh_range_bench_test.go line 225 at r1 (raw file):
readOnlyStr = "_readonly" } loc := fmt.Sprintf("refresh_range_bench_data_%s_%s%s_%d_%d_%d",
nit: should there be another underscore delimiter too?
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/batcheval/cmd_refresh_range_bench_test.go line 225 at r1 (raw file):
Previously, jbowens (Jackson Owens) wrote…
nit: should there be another underscore delimiter too?
the %s without a preceding delimitier is readOnlyStr which is either empty or _readonly. I just added a %s_ in front
|
This was meant to be a moonshot but apparently it might be possible for these fixtures to survive between builds in the CI bazel sandbox. I will expand the PR to include other similar benchmarks. |
Some storage-related benchmarks leave fixtures around in `.gitignore`d directories. There is potential for fixtures from old versions to cause failures. We are seeing some CI failures that would be explained by this (we're not sure yet if it's possible). In any case, it is better to include the version in the fixture name to avid this (which can be a problem even locally). The logging around the fixture location is also improved. Informs cockroachdb#97061 Release note: None Epic: none
RaduBerinde
force-pushed
the
refresh-range-potential-fix
branch
from
1ea9b89
to
204ad97
Compare
RaduBerinde
changed the title
|
Made a similar change to other benchmarks that do this. |
There was a problem hiding this comment.
Reviewed 8 of 8 files at r2.
Reviewable status:
|
bors r+ |
|
TFTR! |
|
Build succeeded: |
This benchmark is sometimes failing in CI and I cannot reproduce locally. The error is consistent with a leftover fixture from an older version (which shouldn't happen because CI should be cleaning up the repo).
This change adds the version to the fixture name so that this kind of cross-version problem cannot occur. This is a good idea regardless of the CI issue.
Informs #97061
Release note: None
Epic: none