feat(mysql-cdc): make `server.id` in WITH option become optional #13031

StrikeW · 2023-10-24T10:12:45Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Generate the server.id for mysql-cdc connector if user doesn't provide to us

related: #12025

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

MySQL CDC

make the server.id option become optional, if user don't provide it we will generate a random id internally.

…ncement

src/meta/service/src/ddl_service.rs

ci/scripts/e2e-source-test.sh

tabVersion · 2023-10-25T03:32:44Z

a little confused, this pr does not fix #12025 case that it does not handle a wrong server.id.

StrikeW · 2023-10-25T03:41:59Z

a little confused, this pr does not fix #12025 case that it does not handle a wrong server.id.

Sorry for the confusion, ~~this pr should deprecate the server.id option, that is we will generate it internally on behalf users.~~
see #13031 (comment)

…ncement

gitguardian · 2023-10-26T02:48:38Z

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	Secret	Commit	Filename
7648795	Generic CLI Secret	`c8f2a13`	integration_tests/iceberg-cdc/run_test.sh	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secret safely. Learn here the best practices.
Revoke and rotate this secret.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}

codecov · 2023-10-26T03:11:09Z

Codecov Report

Merging #13031 (c305768) into main (b724be7) will decrease coverage by 0.07%.
Report is 10 commits behind head on main.
The diff coverage is 2.77%.

@@            Coverage Diff             @@
##             main   #13031      +/-   ##
==========================================
- Coverage   68.40%   68.33%   -0.07%     
==========================================
  Files        1496     1496              
  Lines      252049   252061      +12     
==========================================
- Hits       172413   172252     -161     
- Misses      79636    79809     +173

Flag	Coverage Δ
rust	`68.33% <2.77%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/connector/src/source/base.rs	`69.36% <ø> (ø)`
src/source/src/connector_source.rs	`68.37% <100.00%> (ø)`
src/connector/src/source/cdc/source/reader.rs	`0.00% <0.00%> (ø)`
src/connector/src/macros.rs	`48.00% <0.00%> (-2.00%)`	⬇️
src/connector/src/source/cdc/mod.rs	`13.20% <0.00%> (-2.01%)`	⬇️
src/meta/service/src/ddl_service.rs	`0.00% <0.00%> (ø)`

... and 18 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

hzxa21

Rest LGTM

src/meta/service/src/ddl_service.rs

hzxa21 · 2023-10-26T07:50:53Z

src/meta/service/src/ddl_service.rs

+        visit_fragment(fragment, |node_body| {
+            if let NodeBody::Source(source_node) = node_body {
+                let props = &mut source_node.source_inner.as_mut().unwrap().properties;
+                props.insert("server.id".to_string(), rand_server_id.to_string());


Will this overwrite the user-specified server.id if they set it already?

After consideration, I think we should still allow user to set the ID by their own, so I will revert here back to generate a server.id when it is not provided.
Btw I think it doesn't make sense to check the uniqueness of server.id in the validation, since we cannot lock the server.id to avoid other connector to use it at the time we create a connector. IMO we should provide some ways to improve observability for this run time errors.

StrikeW added 3 commits October 23, 2023 22:33

WIP: generate unique server.id on internaly

96f11ce

add test case

2f95249

WIP: modify source properties on meta

90406ba

github-actions bot added the type/feature label Oct 24, 2023

StrikeW added ci/run-e2e-test type/feature ci/run-build ci/run-build-other and removed type/feature ci/run-build ci/run-build-other labels Oct 24, 2023

StrikeW marked this pull request as ready for review October 24, 2023 10:17

StrikeW added 5 commits October 24, 2023 18:37

fix

61af781

fix

b6e5633

minor

eebf0f6

Merge remote-tracking branch 'origin/main' into siyuan/mysql-cdc-enha…

08c1ac1

…ncement

try fix e2e test

68dfb20

StrikeW requested review from hzxa21, tabVersion, yezizp2012 and wenym1 October 25, 2023 03:06

hzxa21 reviewed Oct 25, 2023

View reviewed changes

src/meta/service/src/ddl_service.rs Outdated Show resolved Hide resolved

tabVersion reviewed Oct 25, 2023

View reviewed changes

ci/scripts/e2e-source-test.sh Outdated Show resolved Hide resolved

StrikeW changed the title ~~feat(mysql-cdc): make server.id option to be optional~~ feat(mysql-cdc): deprecate server.id WITH option and generate it internally Oct 25, 2023

StrikeW added 3 commits October 25, 2023 11:50

deprecate server.id

f9ab934

generage random id for server.id

1bd03db

Merge remote-tracking branch 'origin/main' into siyuan/mysql-cdc-enha…

c8f2a13

…ncement

StrikeW requested a review from a team as a code owner October 26, 2023 02:48

StrikeW requested review from hzxa21 and tabVersion October 26, 2023 03:24

minor

4563622

StrikeW force-pushed the siyuan/mysql-cdc-enhancement branch from 099914b to 4563622 Compare October 26, 2023 03:25

hzxa21 approved these changes Oct 26, 2023

View reviewed changes

generate server.id when it is not provided

c305768

StrikeW mentioned this pull request Oct 26, 2023

Risingwave doesn't throw error when server.id in mysql cdc is incorrect. #12025

Closed

StrikeW enabled auto-merge October 26, 2023 08:42

StrikeW added this pull request to the merge queue Oct 26, 2023

Merged via the queue into main with commit b0f266b Oct 26, 2023
6 of 8 checks passed

StrikeW deleted the siyuan/mysql-cdc-enhancement branch October 26, 2023 09:36

StrikeW changed the title ~~feat(mysql-cdc): deprecate server.id WITH option and generate it internally~~ feat(mysql-cdc): make server.id WITH option as optional Oct 26, 2023

StrikeW changed the title ~~feat(mysql-cdc): make server.id WITH option as optional~~ feat(mysql-cdc): make server.id in WITH option become optional Oct 26, 2023

xxchan mentioned this pull request Nov 10, 2023

risingwave 1.4.0 risingwavelabs/homebrew-risingwave#17

Closed

xxchan mentioned this pull request Oct 29, 2024

refactor: move server.id for MySQL CDC from meta to fe #19169

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(mysql-cdc): make `server.id` in WITH option become optional #13031

feat(mysql-cdc): make `server.id` in WITH option become optional #13031

StrikeW commented Oct 24, 2023 •

edited

Loading

tabVersion commented Oct 25, 2023

StrikeW commented Oct 25, 2023 •

edited

Loading

gitguardian bot commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading

hzxa21 left a comment

hzxa21 Oct 26, 2023

StrikeW Oct 26, 2023

feat(mysql-cdc): make server.id in WITH option become optional #13031

feat(mysql-cdc): make server.id in WITH option become optional #13031

Conversation

StrikeW commented Oct 24, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

tabVersion commented Oct 25, 2023

StrikeW commented Oct 25, 2023 • edited Loading

gitguardian bot commented Oct 26, 2023 • edited Loading

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

codecov bot commented Oct 26, 2023 • edited Loading

Codecov Report

hzxa21 left a comment

Choose a reason for hiding this comment

hzxa21 Oct 26, 2023

Choose a reason for hiding this comment

StrikeW Oct 26, 2023

Choose a reason for hiding this comment

feat(mysql-cdc): make `server.id` in WITH option become optional #13031

feat(mysql-cdc): make `server.id` in WITH option become optional #13031

StrikeW commented Oct 24, 2023 •

edited

Loading

StrikeW commented Oct 25, 2023 •

edited

Loading

gitguardian bot commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading