Add more options to the BulkDumping workload to support restoring a full database in a test. - #14086
Conversation
…ull database in a test.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
saintstack
left a comment
There was a problem hiding this comment.
Looking good. A few things in the below probably worth fixing 'cos will save you headache this is used over in the internal test harness.
| BulkDumpState bulkDumpJob, | ||
| KeyRangeRef bulkDumpJobRange, | ||
| std::map<Key, Value> kvs) { | ||
| bool bulkDumpRangeContainBulkLoadRange = true; // Will set to false if the bulk load job range is not |
There was a problem hiding this comment.
If we skip generateInitialData, this could end up as set incorrectly....
Do bulkDumpRangeContainBulkLoadRange = bulkDumpJobRange.contains(bulkLoadJobRange); ?
There was a problem hiding this comment.
That's a good point, and I'm not sure how best to handle the different flags in their interaction with that check. With the current logic in this PR, when we skip generateInitialData, it also skips any check on the ranges of the restored data, and that's not ideal. However, the current implementation of this check relies on having the original KV data in the kvs map, and that doesn't get populated when generateInitialData is false, and is probably not feasible to populate when we're running on a non-empty database with a large amount of data. Given that, I'm leaning toward having those checks get skipped, and having bulkDumpRangeContainBulkLoadRange left set to true. What do you think?
There was a problem hiding this comment.
Whatever suits you John. You are taking this dumb workload made for a particular simple run and trying to backfill steps/options for use in a different context. You have a +1 from @sbodagala ... go for it.
| runLoad = getOption(options, "runLoad"_sr, true); | ||
| generateInitialData = getOption(options, "generateInitialData"_sr, true); | ||
| sourceJobId = getOption(options, "sourceJobId"_sr, ""_sr).toString(); | ||
|
|
There was a problem hiding this comment.
If we skip the dump, source jobid shouldn't be empty... Its an error if it is...Means one wasn't passed. Add an assert?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS 14.x
|
This PR adds more options to the BulkDumping workload, in order to support dumping and loading full databases, and loading a database from a prior dump. I think this will be useful when running longer workloads through the multitest harness.
This leave the existing functionality intact, and I did a quick simulation run to verify: