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: fix multiple owners exist when owner campaign key is deleted #1072

Merged
merged 8 commits into from
Nov 19, 2020

Conversation

amyangfei
Copy link
Contributor

What problem does this PR solve?

Fix https://github.com/pingcap/ticdc/issues/1044

In some cases, if the campaign owner key is deleted, the owner itself doesn't aware of it and continues running the owner routines. While at the same time, another capture will campaign to be the new owner. In this scenario, multiple owners could exist.

What is changed and how it works?

Add a background campaign key watcher in the owner background.

Check List

Tests

  • Unit test
  • Integration test

Release note

  • Fix the bug that multiple owners could exist when owner campaign key is deleted

@amyangfei amyangfei added type/bugfix This PR fixes a bug. needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. labels Nov 12, 2020
@amyangfei amyangfei added this to the v4.0.9 milestone Nov 13, 2020
@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei
Copy link
Contributor Author

/run-kafka-tests

@amyangfei amyangfei added the status/ptal Could you please take a look? label Nov 13, 2020
cdc/owner.go Show resolved Hide resolved
return cerror.ErrOwnerCampaignKeyDeleted.GenWithStackByArgs()
}
restart:
wch := o.etcdClient.Client.Watch(ctx, key, clientv3.WithRev(resp.Header.Revision+1))
Copy link
Member

Choose a reason for hiding this comment

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

Why +1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change with revision resp.Header.Revision is not needed, so +1

Copy link
Member

Choose a reason for hiding this comment

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

Could you add some comments?

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 19, 2020
@liuzix
Copy link
Contributor

liuzix commented Nov 19, 2020

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

/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

@ti-srebot
Copy link
Contributor

@amyangfei merge failed.

@zier-one
Copy link
Contributor

/run-kafka-tests

cdc/owner.go Show resolved Hide resolved
@amyangfei
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 1078
  • 1081

@amyangfei
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 1078
  • 1081

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@amyangfei merge failed.

@ti-srebot
Copy link
Contributor

/run-all-tests

@zier-one
Copy link
Contributor

/run-integration-tests

@ti-srebot
Copy link
Contributor

@amyangfei merge failed.

@zier-one zier-one merged commit d2df24c into pingcap:master Nov 19, 2020
ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Nov 19, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #1104

@amyangfei amyangfei deleted the fix-multi-owner branch November 19, 2020 09:26
amyangfei pushed a commit that referenced this pull request Nov 20, 2020
@amyangfei amyangfei added the priority/P0 The issue has P0 priority. label Mar 22, 2021
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. priority/P0 The issue has P0 priority. 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
Development

Successfully merging this pull request may close these issues.

Update changefeed config does not work as expected
5 participants