-
Notifications
You must be signed in to change notification settings - Fork 591
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: move server.id for MySQL CDC from meta to fe #19169
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
0399933
to
fa620ec
Compare
9f4c672
to
2cab4a1
Compare
2cab4a1
to
f59bc27
Compare
motivation is to simplify create stream job process in meta Signed-off-by: xxchan <[email protected]>
@@ -1465,6 +1466,14 @@ pub fn bind_connector_props( | |||
.to_string(), | |||
); | |||
} | |||
if !is_create_source && with_properties.is_mysql_cdc_connector() { | |||
// Generate a random server id for mysql cdc source if needed | |||
// `server.id` (in the range from 1 to 2^32 - 1). This value MUST be unique across whole replication |
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.
How do we ensure uniqueness by generating a random number locally?
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.
We don't guarantee uniqueness, this is just a side note.
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.
this is just a side note
I don't quite get this. So will this be a problem?
Signed-off-by: xxchan <[email protected]>
f59bc27
to
06fd5cb
Compare
@@ -45,7 +45,6 @@ create source s with ( | |||
username = 'shared-cdc', | |||
password = 'abcdef', | |||
database.name = 'testdb1', | |||
server.id = '114514' |
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.
Manually tested that before this PR, this test will fail if server.id
not specified
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
motivation is to simplify create stream job process in meta (prepare for #19171, where we will completely remove
fill_table_stream_graph_info
)Also made a behavior change: previously, auto assign
server.id
will only for legacyCREATE TABLE
CDC, but not sharedCREATE SOURCE
.previous work: #13031
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.