Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(query): alter table cluster key #5718

Merged
merged 13 commits into from
Jun 6, 2022
Merged

feat(query): alter table cluster key #5718

merged 13 commits into from
Jun 6, 2022

Conversation

zhyass
Copy link
Member

@zhyass zhyass commented Jun 1, 2022 •
edited
Loading

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Summary about this PR

Changelog

  • New Feature

Related Issues

Fixes #5719

@vercel
Copy link

vercel bot commented Jun 1, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
databend ✅ Ready (Inspect) Visit Preview Jun 6, 2022 at 4:36AM (UTC)

@zhyass zhyass marked this pull request as draft June 1, 2022 05:38
@mergify
Copy link
Contributor

mergify bot commented Jun 1, 2022

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@mergify mergify bot added the pr-feature this PR introduces a new feature to the codebase label Jun 1, 2022
@zhyass zhyass mentioned this pull request Jun 1, 2022
Closed
8 tasks
@zhyass zhyass added the A-storage Area: databend storage label Jun 2, 2022
@zhyass zhyass marked this pull request as ready for review June 4, 2022 15:24
drmingdrmer
drmingdrmer reviewed Jun 4, 2022
View reviewed changes
common/meta/app/src/schema/table.rs Outdated Show resolved Hide resolved
common/proto-conv/tests/it/proto_conv.rs Outdated Show resolved Hide resolved
@drmingdrmer
Copy link
Member

Nice shot!

@drmingdrmer
Copy link
Member

I found there is a flaw in the compatibility:

The updated protobuf TableMeta has changed. The protobuf types should be serialized with a new ver.
But the data written by a query with new ver can not be read by a query with the old ver,
when only some of the query-nodes in a cluster are upgraded.

The crate proto-conv has to write the OLDEST_COMPATIBLE_VER into protobuf message, so that an old query will be able to find out if it is safe to read the data.

https://github.com/datafuselabs/databend/blob/25531a724d38fd242a03155fbfea63e443b68a03/common/proto-conv/src/util.rs#L17-L18

@drmingdrmer drmingdrmer mentioned this pull request Jun 5, 2022
Closed
@drmingdrmer
Copy link
Member

This PR has to merge this patch first:

And it'd better increase VER to a higher value e.g. 2 to indicate meta changes, after #5785 is merged.

@zhyass
Copy link
Member Author

zhyass commented Jun 5, 2022

And it'd better increase VER to a higher value e.g. 2 to indicate meta changes, after #5785 is merged.

Do any changes need to be made in this pr, after #5785 is merged?

@drmingdrmer
Copy link
Member

And it'd better increase VER to a higher value e.g. 2 to indicate meta changes, after #5785 is merged.

Do any changes need to be made in this pr, after #5785 is merged?

It would be better to upgrade the VER every time proto-conv crate changes.
https://github.com/datafuselabs/databend/blob/25531a724d38fd242a03155fbfea63e443b68a03/common/proto-conv/src/util.rs#L17-L18

But the data with a new VER can not be read by query-node without #5785

Update the VER is not mandatory if you do not need to differentiate the version in this PR and before this PR. It is totally OK. :)

@drmingdrmer
Copy link
Member

And it'd better increase VER to a higher value e.g. 2 to indicate meta changes, after #5785 is merged.

Do any changes need to be made in this pr, after #5785 is merged?

Hmm... If you do not upgrade the VER you do not need to wait for my PR: #5678 to merge this one. 🤔

@zhyass
Copy link
Member Author

zhyass commented Jun 5, 2022 •
edited
Loading

Hmm... If you do not upgrade the VER you do not need to wait for my PR: #5678 to merge this one. 🤔

Got it, Thanks :)

dantengsky
dantengsky approved these changes Jun 6, 2022
View reviewed changes
Copy link
Member

@dantengsky dantengsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify mergify bot merged commit 0c0ab5e into databendlabs:main Jun 6, 2022
andylokandy
andylokandy reviewed Jun 10, 2022
View reviewed changes
common/meta/app/src/schema/table.rs
Comment on lines +177 to +179
pub cluster_key: Option<String>,
// The vector of cluster keys.
pub cluster_keys: Vec<String>,
Copy link
Collaborator

@andylokandy andylokandy Jun 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there duplicated cluster_key and cluster_keys?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @zhyass

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there duplicated cluster_key and cluster_keys?

The cluster_key records the current cluster key of the table. The cluster_keys records all historical cluster keys of the table.

@zhyass zhyass mentioned this pull request Jun 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andylokandy andylokandy andylokandy left review comments

@sundy-li sundy-li sundy-li left review comments

@dantengsky dantengsky dantengsky approved these changes

@drmingdrmer drmingdrmer drmingdrmer approved these changes

Assignees
No one assigned
Labels
A-storage Area: databend storage need-review pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Support alter table cluster key
6 participants
@zhyass @drmingdrmer @sundy-li @andylokandy @dantengsky @databend-bot