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

conflict detector(cdc): fix potential dead lock when a transaction generated multiple same hash #10335

Merged
merged 7 commits into from
Dec 27, 2023
Merged

conflict detector(cdc): fix potential dead lock when a transaction generated multiple same hash #10335

merged 7 commits into from
Dec 27, 2023

Conversation

zhangjinpeng87
Copy link
Contributor

@zhangjinpeng87 zhangjinpeng87 commented Dec 21, 2023 •
edited
Loading

What problem does this PR solve?

Issue Number: close #10334

What is changed and how it works?

  • Deduplicate the generated hash in a single transaction to prevent self cyclic dependency in the conflict detector.
  • Refine the conflict detector, make it more readable

Check List

Tests

  • Unit test

Questions

Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?

Release note 

Fix sink-to-mysql checkpoint-ts stuck issue in some rare cases.

@ti-chi-bot ti-chi-bot bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 21, 2023
@ti-chi-bot ti-chi-bot bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 21, 2023
@zhangjinpeng87 zhangjinpeng87 self-assigned this Dec 21, 2023
@zhangjinpeng87 zhangjinpeng87 added the type/bugfix This PR fixes a bug. label Dec 21, 2023
@zhangjinpeng87
make func name more clear
c98c34f 
Signed-off-by: zhangjinpeng1987 <[email protected]>
@zhangjinpeng87
fmt code and comments
0ea9d6e 
Signed-off-by: zhangjinpeng1987 <[email protected]>
@zhangjinpeng87 zhangjinpeng87 changed the title conflict detector: fix potential dead lock when a transaction generated multiple same hash conflict detector(cdc): fix potential dead lock when a transaction generated multiple same hash Dec 21, 2023
@asddongmen asddongmen self-requested a review December 21, 2023 05:08
hicqu
hicqu reviewed Dec 21, 2023
View reviewed changes
pkg/causality/internal/node.go Outdated Show resolved Hide resolved
pkg/causality/internal/node.go Outdated Show resolved Hide resolved
@zhangjinpeng87
address comments
4925e02 
Signed-off-by: zhangjinpeng1987 <[email protected]>
@zhangjinpeng87
fix ci verify
0912041 
Signed-off-by: zhangjinpeng1987 <[email protected]>
@zhangjinpeng87 zhangjinpeng87 mentioned this pull request Dec 21, 2023
Open
20 tasks
@zhangjinpeng87 zhangjinpeng87 added affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. and removed affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. labels Dec 21, 2023
@zhangjinpeng87
Copy link
Contributor Author

/run-check-issue-triage-complete

@zhangjinpeng87
Copy link
Contributor Author

/test verify

@zhangjinpeng87
Copy link
Contributor Author

/test cdc-integration-storage-test

@zhangjinpeng87
Copy link
Contributor Author

/test verify

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Dec 22, 2023
@ti-chi-bot ti-chi-bot bot added the lgtm label Dec 22, 2023
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Dec 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: asddongmen, sdojjy

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Dec 22, 2023
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Dec 22, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-12-22 08:18:34.210907218 +0000 UTC m=+1208205.248134140: ☑️ agreed by asddongmen.
  • 2023-12-22 08:22:18.144241676 +0000 UTC m=+1208429.181468602: ☑️ agreed by sdojjy.

@asddongmen
Copy link
Contributor

/retest

@asddongmen
Copy link
Contributor

Please merge the latest master branch into this pull request to ensure that the failed unit test can pass.

@zhangjinpeng87
Copy link
Contributor Author

/test verify

1 similar comment
@zhangjinpeng87
Copy link
Contributor Author

/test verify

@asddongmen
Copy link
Contributor

/retest

@asddongmen
Copy link
Contributor

@3AceShowHand fixed the flaky unit test in #10327. You can update this PR when #10327 has been merged to pass the test.

@zhangjinpeng87
Copy link
Contributor Author

/test dm-integration-test

@ti-chi-bot ti-chi-bot bot merged commit 5ce4090 into pingcap:master Dec 27, 2023
14 checks passed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-6.5: #10367.

This was referenced Dec 27, 2023
Merged
Merged
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-7.1: #10368.

@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-7.5: #10369.

@ti-chi-bot ti-chi-bot mentioned this pull request Dec 27, 2023
Merged
ti-chi-bot bot pushed a commit that referenced this pull request Dec 28, 2023
@ti-chi-bot
conflict detector(cdc): fix potential dead lock when a transaction ge…
39aa61a 
…nerated multiple same hash (#10335) (#10367)

close #10334
ti-chi-bot bot pushed a commit that referenced this pull request Feb 2, 2024
@ti-chi-bot
conflict detector(cdc): fix potential dead lock when a transaction ge…
b184ae0 
…nerated multiple same hash (#10335) (#10369)

close #10334
ti-chi-bot bot pushed a commit that referenced this pull request Feb 26, 2024
@ti-chi-bot
conflict detector(cdc): fix potential dead lock when a transaction ge…
1b3f140 
…nerated multiple same hash (#10335) (#10368)

close #10334
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CharlesCheung96 CharlesCheung96 CharlesCheung96 left review comments

@hicqu hicqu hicqu left review comments

@asddongmen asddongmen asddongmen approved these changes

@sdojjy sdojjy sdojjy approved these changes

@3AceShowHand 3AceShowHand Awaiting requested review from 3AceShowHand

Assignees

@zhangjinpeng87 zhangjinpeng87

Labels
approved lgtm needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
6 participants
@zhangjinpeng87 @asddongmen @ti-chi-bot @sdojjy @hicqu @CharlesCheung96