Use cluster state data to check concurrent backup/restore#45982
Merged
Use cluster state data to check concurrent backup/restore#45982
Conversation
Implementation: * BackupWorker checks the if any backup/restore which has a path in zookeeper has status not completed, if yes, new backup/restore is stopped. * For not on cluster only active backup / restore is checked. * Removed restore_uuid from RestoreSettings, as it is no longer used.
robot-ch-test-poll3
added
the
pr-improvement
vitlibar
reviewed
Feb 8, 2023
vitlibar
reviewed
Feb 8, 2023
vitlibar
reviewed
Feb 8, 2023
…re Coordination - Use cluster state data to check concurrent backup/restore
…cluster state data to check concurrent backup/restore
… - Use cluster state data to check concurrent backup/restore
…ath - Use cluster state data to check concurrent backup/restore
vitlibar
reviewed
Feb 14, 2023
vitlibar
reviewed
Feb 14, 2023
src/Backups/BackupsWorker.cpp
Outdated
| { | ||
| String root_zk_path = context->getConfigRef().getString("backups.zookeeper_path", "/clickhouse/backups"); | ||
| restore_settings.coordination_zk_path = root_zk_path + "/restore-" + toString(UUIDHelpers::generateV4()); | ||
| restore_settings.coordination_zk_path = root_zk_path + "/restore-" + toString(restore_id); |
Member
There was a problem hiding this comment.
restore_id is something which could be specified by user. It can be the same for two separate restores if they're not simultaneous. And coordination_zk_path must be always unique, that's why it's better to use a random UUID here.
Member
Author
There was a problem hiding this comment.
updated to use restore_uuid, which is generated.
Member
Author
There was a problem hiding this comment.
As you mentioned restore_id can be specified by user, I believe we use this from restore_settings
ClickHouse/src/Backups/BackupsWorker.cpp
Lines 388 to 395 in 0506d92
In that case if
restore_settings.id is not empty restore_id will be the one specified by user, then we will have 2 restores will same restore_id in the map. Seems like this is an issue. Correct me if I am wrong.
vitlibar
reviewed
Feb 14, 2023
…tion_path and added uuid in settings - Use cluster state data to check concurrent backup/restore
Member
Author
vitlibar
approved these changes
Feb 21, 2023
vitlibar
deleted the
Cluster_state_for_disallow_concurrent_backup_restore
branch
This was referenced Feb 22, 2023
robot-ch-test-poll1
added
the
pr-backports-created
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Use cluster state data to check concurrent backup/restore
When concurrent backup/restores are disabled on cluster, with current implementation its possible that 2 nodes start making a backup/restore and can either conflict or not. To ensure that a cluster only runs 1 backup/restore at a time, we use cluster state data from zookeeper.
Implementation: