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
feat: alter column for table with connector #12164
Conversation
Codecov Report
@@ Coverage Diff @@
## main #12164 +/- ##
==========================================
- Coverage 69.78% 69.75% -0.04%
==========================================
Files 1409 1409
Lines 235804 235916 +112
==========================================
Hits 164553 164553
- Misses 71251 71363 +112
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -483,13 +483,19 @@ impl GlobalStreamManager { | |||
|
|||
let dummy_table_id = table_fragments.table_id(); | |||
|
|||
let init_split_assignment = self |
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.
What's the purpose of these code? Would you mind adding some comments here?
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.
+1. Do we have to do this during Replace
? Should the splits be already allocated when creating the altered (dummy) table?
Need help from source experts! cc @tabVersion @shanicky
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.
Yes we need to do this since for source executors, the split info is only fetched via barriers. And for Replace
, the barrier is the Update
mutation from ReplaceTable
, which means it needs the split info to make source executors start to read. pre_allocate_splits
does two things: fetch latest discovered splits and allocate.
Splits are not allocated until this line IIUC.
I think this line is just like what we do in creating table👀
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.
since we change the fragment and it seems to be a new entry for source info, we need to call this func to persist meta data into etcd.
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 LGTM
@@ -483,13 +483,19 @@ impl GlobalStreamManager { | |||
|
|||
let dummy_table_id = table_fragments.table_id(); | |||
|
|||
let init_split_assignment = self |
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.
+1. Do we have to do this during Replace
? Should the splits be already allocated when creating the altered (dummy) table?
Need help from source experts! cc @tabVersion @shanicky
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.
I think the pr is good enough. lets merge
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Support alter add column for table with non-schema-registry source. Syntax is
Idea is simply rebuilding the source executor with latest split info from source manager.
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.