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

kvclient: fix CRTs less than resolvedTs in puller #927

Merged
merged 5 commits into from
Sep 7, 2020

Conversation

zier-one
Copy link
Contributor

@zier-one zier-one commented Sep 4, 2020

What problem does this PR solve?

The resolvedTs may be reversed in the following cases:

  1. There are RegionA and RegionB listened by TiCDC
  2. RegionA and RegionB have merged to RegionAB, the checkpointTS is 5.
  3. The RegionA failed message are sent to the error channel, the checkpointTS in RegionA failed message is 5.
  4. And then, the RegionB failed message are sent to the error chennal, the checkpointTS in RegionB failed message is 5.
  5. The error handler receives the RegionA failed message, rescan the ResionA, create a connection to listen RegionAB with 5 as startTs.
  6. The connection was broken for some reason, the checkpointTs is 10.
  7. And then, the ResionB failed message are received by The error handler, the handler rescan the RegionB, create a connection to listen RegionAB with 5 as startTs.

In steps 6 and 7, the ResolvedTS is regressed from 10 to 5.

What is changed and how it works?

In steps 7, use the checkpointTs record in range lock instead of which in failed message.

Check List

Tests

  • Unit test
  • Integration test

Release note

@zier-one
Copy link
Contributor Author

zier-one commented Sep 4, 2020

/run-integration-tests

@zier-one zier-one changed the title just for testing kvclient: fix CRTs less than resolvedTs in puller Sep 7, 2020
@zier-one zier-one marked this pull request as ready for review September 7, 2020 03:43
Copy link
Contributor

@MyonKeminta MyonKeminta left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ti-srebot
Copy link
Contributor

@MyonKeminta,Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments.See the corresponding SIG page for more information. Related SIG: tools(slack).

@zier-one
Copy link
Contributor Author

zier-one commented Sep 7, 2020

/run-integration-tests

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2020

Codecov Report

Merging #927 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #927   +/-   ##
===========================================
  Coverage   33.8970%   33.8970%           
===========================================
  Files            99         99           
  Lines         12004      12004           
===========================================
  Hits           4069       4069           
  Misses         7534       7534           
  Partials        401        401           

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.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 7, 2020
@overvenus overvenus added the type/bugfix This PR fixes a bug. label Sep 7, 2020
@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 7, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 7, 2020
@overvenus
Copy link
Member

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 7, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@zier-one
Copy link
Contributor Author

zier-one commented Sep 7, 2020

/run-integration-tests

@ti-srebot
Copy link
Contributor

@leoppro merge failed.

@zier-one zier-one merged commit 62d7075 into master Sep 7, 2020
@overvenus overvenus deleted the leoppro/fatal_debug branch September 7, 2020 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants