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

owner: check if the checkpoint ts is less then gc safe time when create the changefeed #1045

Merged
merged 20 commits into from
Nov 19, 2020

Conversation

zier-one
Copy link
Contributor

@zier-one zier-one commented Oct 29, 2020

What problem does this PR solve?

check if the checkpoint ts is less then gc safe time when create the changefeed
fix #1014

Blocked By tikv/pd#3128 tikv/pd#3135 tikv/pd#3136 tikv/pd#3173

Check List

Tests

Release note

  • No release note

@zier-one zier-one added the needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. label Oct 29, 2020
@zier-one zier-one added this to the v4.0.9 milestone Oct 29, 2020
@zier-one zier-one added the status/ptal Could you please take a look? label Oct 29, 2020
@zier-one
Copy link
Contributor Author

/run-integration-tests

@zier-one zier-one added the DNM label Nov 2, 2020
@overvenus
Copy link
Member

Could you move this forward? tikv/pd#3136 does not block this fix I think.

@zier-one
Copy link
Contributor Author

zier-one commented Nov 4, 2020

Could you move this forward? tikv/pd#3136 does not block this fix I think.

If tikv/pd#3136 is not fixed, cdc will throw an error when create a changefeed, because cdc can't get gc safetime before the gc worker running first time.

@zier-one zier-one removed the DNM label Nov 12, 2020
@zier-one
Copy link
Contributor Author

/run-all-tests

@zier-one
Copy link
Contributor Author

/run-all-tests

@zier-one
Copy link
Contributor Author

/run-all-tests

@zier-one
Copy link
Contributor Author

@amyangfei PTAL @liuzix

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 Nov 13, 2020
EnableOldValue: false,
CaseSensitive: true,
EnableOldValue: false,
CheckGCSafePoint: true,
Copy link
Member

Choose a reason for hiding this comment

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

Why add flag? I think it must be checked all the time.

Copy link
Contributor Author

@zier-one zier-one Nov 19, 2020

Choose a reason for hiding this comment

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

cause of the bug in PD (tikv/pd#3128), if TiCDC(4.0.9+) tries to connect to a TiKV(which of the version is less than 4.0.8), the GC safe point check will fail. so we add a flag to skip the check

Copy link
Member

Choose a reason for hiding this comment

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

What about checking GC safepoint by cluster version automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not strict I think. the version check is not correct all the time(for example : the version of a binary built in the master branch may be 4.0.x-xxx-dirty)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and multi-version components in one cluster are not recommended for production. I think it's enough that we just make sure that the cdc is able to connect to the old version tikv

@overvenus
Copy link
Member

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 19, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 19, 2020
@zier-one
Copy link
Contributor Author

/merge

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

/run-all-tests

@codecov-io
Copy link

Codecov Report

Merging #1045 (4e198ac) into master (53886d2) will increase coverage by 0.0335%.
The diff coverage is 35.2941%.

@@               Coverage Diff                @@
##             master      #1045        +/-   ##
================================================
+ Coverage   36.4740%   36.5075%   +0.0335%     
================================================
  Files           109        110         +1     
  Lines         11367      11373         +6     
================================================
+ Hits           4146       4152         +6     
+ Misses         6800       6799         -1     
- Partials        421        422         +1     

@ti-srebot ti-srebot merged commit 456f283 into pingcap:master Nov 19, 2020
ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Nov 19, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #1097

ti-srebot added a commit that referenced this pull request Nov 20, 2020
@zier-one zier-one deleted the fix_1014 branch March 1, 2021 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. status/ptal Could you please take a look?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CDC should not work if changefeed's checkpoint is earlier than current gc safepoint
5 participants