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

client: Fix the issue that TiKV reconnecting may make CDC stuck for a while #531

Merged
merged 4 commits into from
May 8, 2020

Conversation

MyonKeminta
Copy link
Contributor

@MyonKeminta MyonKeminta commented May 6, 2020

What problem does this PR solve?

This PR tries to fix the issue about reconnecting TiKV may make CDC stuck for a while, and added some debug logs for investigation.

Check List

Tests

  • Unit test
  • Integration test

Release note

  • Fix the issue that reconnecting to TiKV may cause CDC stuck for a while

@MyonKeminta MyonKeminta added the DNM label May 6, 2020
@MyonKeminta MyonKeminta changed the title [DNM] client: Add more debug logs for investigating issue about reconnecting [DNM] client: Fix the issue that TiKV reconnecting may make CDC stuck for a while May 7, 2020
@MyonKeminta MyonKeminta removed the DNM label May 7, 2020
@MyonKeminta MyonKeminta marked this pull request as ready for review May 7, 2020 13:07
@MyonKeminta MyonKeminta added type/bugfix This PR fixes a bug. component/kv-client TiKV kv log client component. labels May 7, 2020
@MyonKeminta MyonKeminta requested review from amyangfei and 5kbpers May 7, 2020 13:07
@MyonKeminta MyonKeminta changed the title [DNM] client: Fix the issue that TiKV reconnecting may make CDC stuck for a while client: Fix the issue that TiKV reconnecting may make CDC stuck for a while May 7, 2020
@amyangfei
Copy link
Contributor

/run-integration-tests

Comment on lines 133 to 135
// if server1 != nil {
// server1.Stop()
// }
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 remove these comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry.

@codecov-io
Copy link

Codecov Report

Merging #531 into master will increase coverage by 4.2345%.
The diff coverage is 76.9230%.

@@               Coverage Diff                @@
##             master       #531        +/-   ##
================================================
+ Coverage   26.5300%   30.7646%   +4.2345%     
================================================
  Files            71         68         -3     
  Lines          7614       6683       -931     
================================================
+ Hits           2020       2056        +36     
+ Misses         5449       4480       -969     
- Partials        145        147         +2     

@amyangfei amyangfei added the status/ptal Could you please take a look? label May 7, 2020
@sre-bot
Copy link

sre-bot commented May 8, 2020

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

@amyangfei amyangfei added LGT1 and removed status/ptal Could you please take a look? labels May 8, 2020
@amyangfei
Copy link
Contributor

/rebuild

@sre-bot
Copy link

sre-bot commented May 8, 2020

@amyangfei
Copy link
Contributor

/rebuild

@zier-one
Copy link
Contributor

zier-one commented May 8, 2020

LGTM

Copy link
Contributor

@5kbpers 5kbpers left a comment

Choose a reason for hiding this comment

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

LGTM

@5kbpers 5kbpers added LGT2 and removed LGT1 labels May 8, 2020
@amyangfei amyangfei merged commit ed51515 into pingcap:master May 8, 2020
@MyonKeminta MyonKeminta deleted the m/reconnection-debug-logs branch May 8, 2020 14:59
5kbpers pushed a commit to 5kbpers/ticdc that referenced this pull request Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/kv-client TiKV kv log client component. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants