-
Notifications
You must be signed in to change notification settings - Fork 761
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
refactor: simplify alter and drop cluster key logic #17128
Conversation
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.
Reviewed 10 of 29 files at r1, all commit messages.
Reviewable status: 10 of 29 files reviewed, 1 unresolved discussion (waiting on @dantengsky, @SkyFan2002, and @zhyass)
src/query/service/src/interpreters/interpreter_cluster_key_alter.rs
line 82 at r1 (raw file):
.options .insert(OPT_KEY_CLUSTER_TYPE.to_owned(), plan.cluster_type.clone()); new_table_meta = new_table_meta.push_cluster_key(cluster_key_str);
cluster_key_str
is format!("({})", plan.cluster_keys.join(", "));
. Is it meant to push all the keys as a single item?
https://github.com/databendlabs/databend/blob/main/src/meta/app/src/schema/table.rs#L265-L266 The |
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.
Reviewed 2 of 29 files at r1.
Reviewable status: 12 of 29 files reviewed, 1 unresolved discussion (waiting on @dantengsky and @zhyass)
src/query/service/src/interpreters/interpreter_cluster_key_alter.rs
line 89 at r1 (raw file):
new_table_meta, }; catalog.update_single_table_meta(req, table_info).await?;
This update may fail due to parallel updates in other threads, because it just update the table meta with a certain seq.
I'm not quite sure about the logic in update_multi_table_meta()
. It's quite complicated a function. Maybe @SkyFan2002 can tell if it retry on a seq conflict.
And there should be a simple version of update_multi_table_meta
that just update one table meta record, which will make the logic easier to test and understand.
|
2673c1e
to
8eb18e2
Compare
8eb18e2
to
ba4b8e2
Compare
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.
Reviewed 13 of 28 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 23 of 51 files reviewed, 1 unresolved discussion (waiting on @dantengsky and @zhyass)
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
This PR simplifies the logic for altering and dropping cluster keys. Instead of generating a new snapshot, the table metadata is directly updated to reflect the changes. This optimization reduces unnecessary snapshot creation and improves operation efficiency.
Tests
Type of change
This change is