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.

Sign up for GitHub

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

Jump to bottom

mysql_sink: fix premature checkpoint advances during resolved-ts regressions #2033

Closed
wants to merge 8 commits into from
Closed

mysql_sink: fix premature checkpoint advances during resolved-ts regressions #2033

wants to merge 8 commits into from

Conversation

liuzix
Copy link
Contributor

@liuzix liuzix commented Jun 10, 2021 •
edited
Loading

What problem does this PR solve?

  • mysqlSink might report checkpoints prematurely if EmitRowChangedEvents is called with events whose commit-ts is less than what has been last used to call FlushRowChangedEvents. This scenario would not have happened before *: fix the output in changefeed out of order #1247

What is changed and how it works?

  • We dial back the internal resolved-ts in mysqlSink if an earlier event is received.

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Fix a bug that could cause data losses if a TiCDC node is kills immediate after a table migration

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 10, 2021
@liuzix
Copy link
Contributor Author

liuzix commented Jun 10, 2021

/run-all-tests

@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 10, 2021
@liuzix
fix
7a4a4fe
@liuzix
Merge branch 'zixiong-fix-sink-premature-checkpoint' of github.com:li…
1e85bc8 
…uzix/ticdc into zixiong-fix-sink-premature-checkpoint
@liuzix
Copy link
Contributor Author

liuzix commented Jun 10, 2021

/run-all-tests

2 similar comments
@liuzix
Copy link
Contributor Author

liuzix commented Jun 11, 2021

/run-all-tests

@amyangfei
Copy link
Contributor

/run-all-tests

@amyangfei amyangfei added needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. needs-cherry-pick-release-5.1 Should cherry pick this PR to release-5.1 branch. labels Jun 11, 2021
@amyangfei amyangfei added this to the v5.1.0 milestone Jun 11, 2021
@amyangfei amyangfei added the release-blocker This issue blocks a release. Please solve it ASAP. label Jun 11, 2021
@liuzix
Copy link
Contributor Author

liuzix commented Jun 11, 2021

/run-all-tests

1 similar comment
@liuzix
Copy link
Contributor Author

liuzix commented Jun 11, 2021

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Jun 11, 2021

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Jun 11, 2021

/run-integration-tests

@liuzix
Copy link
Contributor Author

liuzix commented Jun 11, 2021

/run-unit-tests

@liuzix
Copy link
Contributor Author

liuzix commented Jun 11, 2021

/run-leak-tests

@liuzix liuzix marked this pull request as ready for review June 11, 2021 10:36
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2021
@liuzix liuzix changed the title sink: fix premature checkpoint advances during resolved-ts regressions mysql_sink: fix premature checkpoint advances during resolved-ts regressions Jun 11, 2021
@liuzix
Copy link
Contributor Author

liuzix commented Jun 11, 2021

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Jun 12, 2021

/run-all-tests

@codecov-commenter
Copy link

Codecov Report

Merging #2033 (a3ae1ab) into master (b6d58ef) will increase coverage by 0.1178%.
The diff coverage is 64.6993%.

@@               Coverage Diff                @@
##             master      #2033        +/-   ##
================================================
+ Coverage   54.0780%   54.1958%   +0.1178%     
================================================
  Files           156        164         +8     
  Lines         16589      17422       +833     
================================================
+ Hits           8971       9442       +471     
- Misses         6667       6963       +296     
- Partials        951       1017        +66

amyangfei
amyangfei reviewed Jun 13, 2021
View reviewed changes
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

By the way, please complete the pr description.

cdc/sink/mysql.go Show resolved Hide resolved
@liuzix liuzix added the status/ptal Could you please take a look? label Jun 15, 2021
@liuzix
Copy link
Contributor Author

liuzix commented Jun 15, 2021

/run-all-tests

@amyangfei amyangfei removed the release-blocker This issue blocks a release. Please solve it ASAP. label Jun 16, 2021
@amyangfei amyangfei modified the milestones: v5.1.0, v5.1.1 Jun 21, 2021
@overvenus overvenus added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed type/dnm labels Jul 8, 2021
@liuzix liuzix closed this Jul 14, 2021
This was referenced Jul 27, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amyangfei amyangfei amyangfei left review comments

@zier-one zier-one Awaiting requested review from zier-one

Assignees
No one assigned
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. needs-cherry-pick-release-5.1 Should cherry pick this PR to release-5.1 branch. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/ptal Could you please take a look?
Projects
None yet
Milestone
v5.1.1
Development

Successfully merging this pull request may close these issues.
5 participants
@liuzix @ti-chi-bot @amyangfei @codecov-commenter @overvenus