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

sink/mysql: fix txn wg counter is not updated correctly #1524

Merged
merged 8 commits into from Mar 26, 2021
Merged

sink/mysql: fix txn wg counter is not updated correctly #1524

merged 8 commits into from Mar 26, 2021

Conversation

amyangfei
Copy link
Contributor

@amyangfei amyangfei commented Mar 18, 2021 •
edited

What problem does this PR solve?

Fix #1486

What is changed and how it works?

There exist two scenarios that txnWg is not updated correctly

  • When a worker receives a new txn from txnCh, and triggers flushRows function, flushRows returns an error and the routine terminates. There could be txns remained in txnCh
  • When a worker exits and fetches all txns from txnCh and update txnWg, there could be new txns sent to txnCh.

So when the worker exits, fill in the txnCh to prevent new txns sent to txnCh, and update the txnWg with correct txn number.

  • Add a *sync.WaitGroup field in SingleTableTxn struct.
  • Before wait for all workers flushed txns, broadcast a control txn, which contains a *sync.Waitgroup field
  • Wait the wait group until all txns in workers are flushed.
  • If error happens in worker, consume all txns and mark wait group done once.

There are two candidate solutions, one is to use txnNum to control, the other one is to introduce the control txn

Check List

Tests

  • Unit test
  • Integration test

Release note

  • Fix a bug that txn counter is not correctly updated which may cause database connection leak.

@amyangfei amyangfei added status/WIP type/bugfix This PR fixes a bug. component/sink Sink component. needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. labels Mar 18, 2021
@amyangfei amyangfei added this to the v5.0.0 milestone Mar 18, 2021
@amyangfei
Copy link
Contributor Author

/run-all-tests

1 similar comment
@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei amyangfei added status/ptal Could you please take a look? needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. and removed status/WIP labels Mar 19, 2021
@amyangfei
Copy link
Contributor Author

PTAL @leoppro @liuzix

@amyangfei amyangfei added the priority/p0 Issue with the highest priority, should be resolved as soon as possible. label Mar 22, 2021
@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei
Copy link
Contributor Author

/run-all-tests

zier-one
zier-one reviewed Mar 23, 2021
View reviewed changes
Copy link
Contributor

@zier-one zier-one 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 Mar 23, 2021
@amyangfei
Copy link
Contributor Author

/run-all-tests

2 similar comments
@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei
Copy link
Contributor Author

/run-all-tests tikv=pr/9872

@amyangfei amyangfei added the release-blocker This issue blocks a release. Please solve it ASAP. label Mar 26, 2021
@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 26, 2021
@liuzix
Copy link
Contributor

liuzix commented Mar 26, 2021

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • liuzix

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 26, 2021
@amyangfei
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 2037275

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 26, 2021
@amyangfei
Copy link
Contributor Author

/merge

@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei
Copy link
Contributor Author

/run-kafka-tests

@ti-chi-bot ti-chi-bot merged commit eb45a5a into pingcap:master Mar 26, 2021
@amyangfei amyangfei deleted the fix-mysql-conn-leak branch March 26, 2021 14:23
ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Mar 26, 2021
@amyangfei @ti-srebot
cherry pick pingcap#1524 to release-4.0
cf96b42 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Mar 26, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #1556

ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Mar 26, 2021
@amyangfei @ti-srebot
cherry pick pingcap#1524 to release-5.0
e804c1e 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Mar 26, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #1557

amyangfei pushed a commit that referenced this pull request Mar 27, 2021
@ti-srebot
sink/mysql: fix txn wg counter is not updated correctly (#1524) #1556
db65db4
amyangfei pushed a commit that referenced this pull request Mar 27, 2021
@ti-srebot
sink/mysql: fix txn wg counter is not updated correctly (#1524) (#1557)
adcc9c6
@amyangfei amyangfei mentioned this pull request Jun 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zier-one zier-one zier-one left review comments

Assignees
No one assigned
Labels
component/sink Sink component. needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. priority/p0 Issue with the highest priority, should be resolved as soon as possible. release-blocker This issue blocks a release. Please solve it ASAP. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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
v5.0.0
Development

Successfully merging this pull request may close these issues.

ErrMySQLConnectionError: 1040: Too many connections
5 participants
@amyangfei @liuzix @ti-chi-bot @ti-srebot @zier-one