New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lock zero copy parts more atomically #49211
Conversation
…kHouse into fix_zero_copy_not_atomic
This is an automated comment for commit 6f3f202 with description of existing statuses. It's updated for the latest CI running
|
You can ask -- what does it mean "more atomically"? Because we still have places where we do it non-atomically, but all merges/mutations/fetches create zero-copy locks atomically with parts commit now. |
@@ -861,6 +868,12 @@ class StorageReplicatedMergeTree final : public MergeTreeData | |||
int32_t mode = zkutil::CreateMode::Persistent, bool replace_existing_lock = false, | |||
const String & path_to_set_hardlinked_files = "", const NameSet & hardlinked_files = {}); | |||
|
|||
static void getZeroCopyLockNodeCreaetOps( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
static void getZeroCopyLockNodeCreaetOps( | |
static void getZeroCopyLockNodeCreateOps( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest all LGTM
Bad cast: #49445 |
logger issues #49420 |
Keeper multinode #49446 |
Really hard to validate this bugfix |
auto error = zookeeper->tryMulti(ops, responses); | ||
if (error == Coordination::Error::ZOK) | ||
size_t failed_op = zkutil::getFailedOpIndex(error, responses); | ||
/// Part was locked before, unfortunately it's possible during moves |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alesapin, did you mean replace/move partition or moves between disks? AFAIU, during replace/move partition, the existing lock is ephemeral...
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix possible dataloss because of non-atomic locking in zero copy replication. Fixes #48474 (comment).