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

owner: prevent table start-ts regressions #1236

Merged
merged 44 commits into from
Jan 22, 2021
Merged

owner: prevent table start-ts regressions #1236

merged 44 commits into from
Jan 22, 2021

Conversation

liuzix
Copy link
Contributor

@liuzix liuzix commented Dec 24, 2020 •
edited
Loading

What problem does this PR solve?

What is changed and how it works?

  • Fixed several bugs in Owner so that never will a local checkpoint be less than the global checkpoint.
  • Cleaned up checkpoint ts semantics with respect to DDL operations, so that a DDL is finished if and only if global checkpoint >= FinishTs.
  • Fixed a bug in processor introduced by *: fix the output in changefeed out of order #1247 which causes local checkpoint regression.
  • Added Etcd state checker to integration test.

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Fix bugs in the owner that could lead to abnormal CDC server exits during table migrations.

@liuzix
Copy link
Contributor Author

liuzix commented Dec 24, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Dec 24, 2020

/run-all-tests

@liuzix liuzix changed the title only for testing: add panics for invariant violations owner: prevent table start-ts regressions Dec 24, 2020
@liuzix liuzix added type/bugfix This PR fixes a bug. component/replica-model Replication model component. status/ptal Could you please take a look? labels Dec 24, 2020
@liuzix liuzix modified the milestones: v4.0.10, 5.0.0-rc Dec 24, 2020
@liuzix liuzix requested a review from zier-one December 24, 2020 17:58
@liuzix liuzix mentioned this pull request Dec 25, 2020
Closed
@liuzix
Copy link
Contributor Author

liuzix commented Dec 25, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Dec 25, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Dec 25, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Dec 25, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Dec 25, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Dec 25, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Dec 28, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Jan 21, 2021

/run-all-tests

@liuzix liuzix added needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. and removed status/WIP labels Jan 21, 2021
@amyangfei amyangfei removed the needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. label Jan 21, 2021
amyangfei
amyangfei reviewed Jan 21, 2021
View reviewed changes
cdc/processor.go
Comment on lines 784 to +786
if replicaInfo.StartTs < globalcheckpointTs {
// use Warn instead of Panic in case that p.globalcheckpointTs has not been initialized.
// The cdc_state_checker will catch a real inconsistency in integration tests.
Copy link
Contributor

@amyangfei amyangfei Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If globalcheckpointTs is larger than 0 (initialized already), and replicaInfo.StartTs < globalcheckpointTs, then it is a bug (not as expected), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@liuzix
Copy link
Contributor Author

liuzix commented Jan 22, 2021

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Jan 22, 2021

/run-all-tests

@amyangfei amyangfei mentioned this pull request Jan 22, 2021
Open
4 tasks
amyangfei
amyangfei reviewed Jan 22, 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.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 22, 2021
@amyangfei
Copy link
Contributor

Please also update document about the behavior change with DDL error after PR is merged. ref item-3 in #1328

@amyangfei
Copy link
Contributor

/run-kafka-tests

@zier-one
Copy link
Contributor

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jan 22, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 22, 2021
@zier-one
Copy link
Contributor

/merge

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

/run-all-tests

@ti-srebot ti-srebot merged commit 78de95e into pingcap:master Jan 22, 2021
ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Jan 22, 2021
@liuzix @ti-srebot
cherry pick pingcap#1236 to release-4.0
eef18d9 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Jan 22, 2021
Closed
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #1331

@liuzix
Copy link
Contributor Author

liuzix commented Jan 27, 2021

/run-cherry-picker

liuzix added a commit to liuzix/ticdc that referenced this pull request Jan 27, 2021
@liuzix
owner: prevent table start-ts regressions (pingcap#1236)
2b1d3d5 
(cherry picked from commit 78de95e)
ti-srebot pushed a commit that referenced this pull request Jan 27, 2021
@liuzix
owner: prevent table start-ts regressions (#1236) (#1352)
54f8a37
@amyangfei amyangfei mentioned this pull request Jan 28, 2021
Closed
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

@ti-srebot ti-srebot ti-srebot approved these changes

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

Assignees
No one assigned
Labels
component/replica-model Replication model component. 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? type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
v4.0.11
Development

Successfully merging this pull request may close these issues.
5 participants
@liuzix @codecov-io @amyangfei @zier-one @ti-srebot