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.

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

[FLINK-35540][cdc-common][cdc-connector][mysql] fix lost table when database and table are with the same name #3396

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

qg-lin
Copy link
Contributor

@qg-lin qg-lin commented Jun 6, 2024

Copy link
Contributor

@yuxiqian yuxiqian left a comment

Choose a reason for hiding this comment

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

Thanks @qg-lin, these changes look good to me! I just left some comments on test cases.

this(
container,
databaseName,
Integer.toUnsignedString(new Random().nextInt(), 36),
withIdentifier ? getIdentifier() : StringUtils.EMPTY,
Copy link
Contributor

Choose a reason for hiding this comment

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

The identifier suffix is meant to avoid test case conflicts since multiple cases might run simultaneously on a single MySQL database instance. It should be fine now since there's only one test case using this class, but might cause trouble in the future if more testcases are added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally I would prefer not to change UniqueDatabase, and some alternative approaches are 1) Keep using UniqueDatabase to test identical DB and table name scenario, create tables during tests or 2) Create another class like CustomDatabase which allows specifying database name directly without appending suffix. Looking forward to hearing @qg-lin's thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I'll rewrite the case.

Copy link
Contributor

@yuxiqian yuxiqian left a comment

Choose a reason for hiding this comment

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

Thanks for @qg-lin's nice work! Also please run mvn spotless:apply to fix format violations before it could be merged.

cc @leonardBang

Copy link
Contributor

@leonardBang leonardBang left a comment

Choose a reason for hiding this comment

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

LGTM

@leonardBang leonardBang merged commit c958daf into apache:master Jun 11, 2024
15 checks passed
wuzhenhua01 pushed a commit to wuzhenhua01/flink-cdc-connectors that referenced this pull request Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants