-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
pd: cancel call when refreshing client #2669
Conversation
can we use failpoint to reproduce it? E.g, force breaking the loop and take the receiver. |
Do we need to call cancel on raft client? |
I think we need. Maybe we should add keep timeout too, but @disksing meets some problems with it. |
I don't think so. If client is refreshed in raft client, then the streaming call is dropped and recreated immediately. |
I don't get it. What loop to be break? And how and why to take the receiver? |
what I mean is not returning Ready and going here https://github.com/BusyJay/tikv/blob/709096a3ec8e0011b6ac4e434e89f3ca2d3a2278/src/pd/util.rs#L70 Is it OK? |
I don't see any connections between what you mention and the bug trying to fix here. Actually the poll method is never called again until cancel is called. |
Got it. |
PTAL |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
/run-integration-tests |
/rebuild |
/run-integration-tests |
* pd: cancel call when refreshing client
* pd: cancel call when refreshing client
This is a quick fix for possible stale heartbeat streaming call.
Streaming call won't report error when messages are dropped silently in network. However there are sync requests in pd client can detect this error by timeout. When the error is detected, client will be refreshed. At that time, streaming call created by old client should be canceled automatically. But this doesn't happen as expected, so this pr cancels it explicitly.
We need to add a test case to tested this with iptables.
The issue needs to be investaged further too. /cc tikv/grpc-rs#150