storageccl: fix and reenable BenchmarkTimeBoundIterate #96771
Conversation
|
This change is |
This benchmark creates a `mvcc_data` directory in the current working dir (i.e. `pkg/ccl/storageccl/engineccl`) and reuses it if exists. Normally this would not be permitted in CI; but `mvcc_data` is in `.gitignore` so it doesn't trip the "assert workspace clean" CI step. The leftover `mvcc_data` can be from an older version, which can cause mysterious CI failures. This fix changes to generating the data in a temporary directory and cleans it up afterwards. The generation only takes a few seconds (significantly less than what it takes to run all benchmarks). Release note: None Epic: none
RaduBerinde
force-pushed
the
fix-iterate-bench
branch
from
36c6798 to
e68c5d6
Compare
There was a problem hiding this comment.
I think we really need some method of caching artifacts between benchmarks. The generated data isn't deterministic, so it can increase the variance of the benchmark to the point that it's difficult to get reliable measurements. This particular benchmark's data is small, but we have others in pkg/storage that may take minutes to construct the initial state.
I'm a little surprised that this is happening now, because under bazel anything written within the sandbox is lost when the sandbox is torn down. We've been running our pkg/storage benchmarks using make bench despite it being deprecated because of it (#83599).
This change LGTM. I think the same problem applies to pkg/storage's MVCC benchmarks though, and I don't think we want to apply the same treatment there.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @bananabrick)
I am not sure but the failures might have been from the non-bazel CI. I could add an env variable that you can use to point it to a less temporary location, would that help? |
|
An alternative would be to change the behavior only under CI.. is there a way we can tell from the code? CC @rickystewart |
Oh, good idea. I think we could change the behavior if |
Isn't |
By the way, the data is generated using a hardcoded random seed. Are there other sources of non-determinism that cause the resulting store to be materially different? (eg timing of operations or IOs)? |
Yeah, but we need some kind of tooling support to make it viable for these benchmarks. Bazel tests today can't write anything outside of the sandbox, so there's no way to generate a fixture and have it accessible by the next run. In #83599 we
Yeah, we allow up to 3 concurrent compactions, 1 flush and 1 table stats collector goroutine. The timing of these 5 routines affect the resulting LSM and what physical keys exist. |
Hm, I see. One way would be to check the fixtures into a special repository and pull them in as a dependency to the benchmark. |
|
Actually, would it be so bad if we checked in |
Seems okay to me, but I bet @rickystewart or others on @cockroach-dev-inf would have an opinion. |
|
I'm finding more benchmarks that apply the same technique, and which occasionally run into fixtures from previous versions on CI: |
|
Closing this for now; filed issue #97061 to track. |
This benchmark creates a
mvcc_datadirectory in the current working dir (i.e.pkg/ccl/storageccl/engineccl) and reuses it if exists. Normally this would not be permitted in CI; butmvcc_datais in.gitignoreso it doesn't trip the "assert workspace clean" CI step.The leftover
mvcc_datacan be from an older version, which can cause mysterious CI failures.This fix changes to generating the data in a temporary directory and cleans it up afterwards. The generation only takes a few seconds (significantly less than what it takes to run all benchmarks).
Release note: None
Epic: none