Removed no-op mode in BackupWorker#12748
Conversation
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
| if (deterministicRandom()->random01() < 0.5) { | ||
| set_config("backup_worker_enabled:=1"); | ||
| } |
There was a problem hiding this comment.
is this related to no-op? If not, will we regress on simulation coverage?
There was a problem hiding this comment.
This config decides whether the backup workers need to be started randomly.
But the backup workers are used only when backup v2 is used. Which backup to use backup (v1/v2) is decided in the specific simulation tests.
Before this change, if backup workers are started, but backup v2 is not running, backup workers are used to run in no-op mode(just discards tlog entries and not upload to S3).
I removed the no-op mode so the code cannot handle backup workers running without backup v2. So I removed this change.
After this change, only if backup v2 starts, it automatically starts the backupWorkers, there is no need to run no-op mode.
Simulation coverage:
Since selection of backup v1 vs v2 is still taken in simulation tests, the coverage should not have affect.
There was a problem hiding this comment.
Makes sense. For my understanding regarding coverage, these are the possible states:
- (backup_v2 = true, backup_worker = true)
- (backup_v2 = true, backup_worker = false)
- (backup_v2 = false, backup_worker = true)
- (backup_v2 = false, backup_worker = false)
(1) and (4) should be supported before and after this change.
I think what you're saying is that with deletion of no-op mode, (3) is an invalid state. So we remove the code that introduces it. That makes sense.
How about (2)? It doesn't sound useful (backup v2 enabled but no backup worker), but is that a valid state i.e. a state we allow? If yes, then sounds like we won't test it because once backup_v2 = true, we automatically do backup_worker = true, which is state (1).
There was a problem hiding this comment.
When backup v2 is enabled, we will do a config change of backup_worker=true. So, we should never be in state of backup_v2 = true, backup_worker = false.
If some reason we are in that state, the backup will be missing some files.
Cherry-pick of #12746
Removed no-op mode in BackupWorker
Simulation run:
20260304-025757-neethu1-7.4-d03ea42930425cc8 compressed=True data_size=60544534 duration=4071720 ended=100000 fail_fast=10 max_runs=100000 pass=100000
Code-Reviewer Section
The general pull request guidelines can be found here.
Please check each of the following things and check all boxes before accepting a PR.
For Release-Branches
If this PR is made against a release-branch, please also check the following:
release-branchormainif this is the youngest branch)