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

[mysql] Snapshot split the chunks asynchronously (ververica#915) #931

Merged
merged 6 commits into from
Mar 17, 2022

Conversation

fuyun2024
Copy link
Contributor

No description provided.

executor.submit(
() -> {
while (remainingTables.size() > 0) {
TableId nextTable = remainingTables.pollFirst();
Copy link
Contributor

Choose a reason for hiding this comment

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

How we make sure the table splits generation has finished if failover happened ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comments. I have revised the code again.

@fuyun2024 fuyun2024 force-pushed the asynchronously-snapshot-spliting branch from 2058b7c to 73e8520 Compare March 15, 2022 07:22
@fuyun2024 fuyun2024 force-pushed the asynchronously-snapshot-spliting branch from c375bdf to 00b97c6 Compare March 15, 2022 07:51
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.

Thanks @fuyun2024 for the contribution, could you also add some unit tests to cover the change?

@@ -180,27 +191,60 @@ private void captureNewlyAddedTables() {
}
}

private void asynchronouslySplitIfNeed() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private void asynchronouslySplitIfNeed() {
private void startaAsynchronouslySplit() {

if (!remainingTables.isEmpty()) {
if (executor == null) {
ThreadFactory threadFactory =
new ThreadFactoryBuilder().setNameFormat("snapshot-split").build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
new ThreadFactoryBuilder().setNameFormat("snapshot-split").build();
new ThreadFactoryBuilder().setNameFormat("snapshot-splitting").build();

}
return getNext();
} else {
return Optional.empty();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we release the executor resource after splitting finished ?

Comment on lines 235 to 237
} catch (InterruptedException e) {
e.printStackTrace();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please throw FlinkRuntimeException with a detail error message.

return getNext();
} else {
return Optional.empty();
} else if (!remainingTables.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a race condition that remainingTables is empty but the asynchronous thread just added the snapshot splits of last table to remainingSplits, and then the last table will be missed.

@fuyun2024 fuyun2024 force-pushed the asynchronously-snapshot-spliting branch 2 times, most recently from f8d9e5b to 0af1600 Compare March 16, 2022 08:32
@fuyun2024 fuyun2024 force-pushed the asynchronously-snapshot-spliting branch from ec6c681 to c8836ba Compare March 17, 2022 07:35
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.

Thanks @fuyun2024 for the contribution, LGTM

@leonardBang leonardBang merged commit 41a5463 into apache:master Mar 17, 2022
@fuyun2024 fuyun2024 deleted the asynchronously-snapshot-spliting branch October 12, 2022 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants