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

tikv: non-blocking establish superbatch connection with timeout (#12733) (#12814) #13277

Merged
merged 3 commits into from Nov 8, 2019

Conversation

sre-bot
Copy link
Contributor

@sre-bot sre-bot commented Nov 8, 2019

cherry-pick #12814 to release-3.1


cherry-pick #12733 to 3.0

without any conflict just fix logger compile error
9f30108

What problem does this PR solve?
fix the potential problem that slow establishes the connection make other kv requests be slow

5 min dial timeout doesn't work in superbatch
createConnArray will block wait conn establish on create tikvClient.BatchCommands
long time hold lock in createConnArray will block getConnArray
What is changed and how it works?
make establish grpc client in other goroutine which is blocking process
check estDone before send request, send directly if done
wait estReady channel if it's not done(only need select channel for first time)
set right dialTimeout for batch client creation
more words:

current impl is using connectivity state API, to wait non-blocking dial result with context timeout.
new but not released grpc, we maybe can use grpc/grpc-go#2960 to simplify impl
also take care grpc/grpc-go#1786 if upgrade to new grpc in future
Check List
Tests

Unit test
Integration test
Code changes

impl
Side effects

n/a
Related changes

Need to cherry-pick to the release branch
Release note

fix the potential problem that slow establishes the connection make other kv requests be slow 

This change is Reviewable

@sre-bot
Copy link
Contributor Author

sre-bot commented Nov 8, 2019

/run-all-tests

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

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

LGTM

@coocood
Copy link
Member

coocood commented Nov 8, 2019

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 8, 2019
@sre-bot
Copy link
Contributor Author

sre-bot commented Nov 8, 2019

/run-all-tests

@sre-bot
Copy link
Contributor Author

sre-bot commented Nov 8, 2019

@sre-bot merge failed.

@jackysp
Copy link
Member

jackysp commented Nov 8, 2019

/run-unit-test

@jackysp jackysp merged commit 5c72688 into pingcap:release-3.1 Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/tikv priority/release-blocker This PR blocks a release. Please review it ASAP. status/can-merge Indicates a PR has been approved by a committer. type/bug-fix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants