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

*: Using the global singleton for pd client and tikv client, and fix pd client freeze #1217

Merged
merged 2 commits into from
Dec 16, 2020

Conversation

zier-one
Copy link
Contributor

@zier-one zier-one commented Dec 16, 2020

What problem does this PR solve?

Using the global singleton for pd client and tikv client, and fix pd client freeze

What is changed and how it works?

The modification of this PR bypasses the bug about pd client freeze by using a global singleton for pd client

Check List

Tests

  • Unit test
  • Integration test

Release note

  • Fix the bug of possible TiCDC server freeze

@zier-one zier-one added needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-5.0 status/ptal Could you please take a look? labels Dec 16, 2020
@zier-one zier-one added this to the v4.0.9 milestone Dec 16, 2020
@overvenus
Copy link
Member

Please add some tests.

@liuzix
Copy link
Contributor

liuzix commented Dec 16, 2020

LGTM. If we do want tests, can we assert that we only have one pdClient globally by comparing pointers?

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 16, 2020
@zier-one zier-one added the type/bugfix This PR fixes a bug. label Dec 16, 2020
@zier-one
Copy link
Contributor Author

/run-integration-tests

@codecov-io
Copy link

Codecov Report

Merging #1217 (da57ce6) into master (7c6c754) will increase coverage by 0.0238%.
The diff coverage is 8.3333%.

@@               Coverage Diff                @@
##             master      #1217        +/-   ##
================================================
+ Coverage   46.2837%   46.3076%   +0.0238%     
================================================
  Files           115        115                
  Lines         11961      11957         -4     
================================================
+ Hits           5536       5537         +1     
+ Misses         5801       5794         -7     
- Partials        624        626         +2     

@overvenus
Copy link
Member

overvenus commented Dec 16, 2020

It's hard to unit test in current code base, can we test it in integration tests via failpoint?

@overvenus
Copy link
Member

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 16, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 16, 2020
@zier-one zier-one merged commit 2a09a89 into master Dec 16, 2020
ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Dec 16, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #1219

ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Dec 16, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #1220

amyangfei pushed a commit that referenced this pull request Dec 16, 2020
amyangfei pushed a commit that referenced this pull request Dec 16, 2020
@zier-one zier-one deleted the fix_pd_hang branch February 3, 2021 06:49
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/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.

5 participants