kv: increase dist sender's RPC timeout #16088
Conversation
|
This change is |
There was a problem hiding this comment.
Yes, I'm not too worried about this change. This is a relatively rare occurrence and I'm not convinced there's going to be a big win to allow these ambiguous results to return after a wait. In those cases, it's likely that the live node (live because gRPC heartbeats are still working) is blocked up in a Raft group or similar. In those cases, there are all kinds of other problems, so letting this pending request continue waiting isn't likely to make the system more responsive.
|
FWIW, I think what we want for the DistSender timeouts is some mechanism to differentiate between slow RPCs that have been accepted by the lease holder versus RPCs that haven't even been accepted yet (and thus we don't really know if the recipient even is the leaseholder). In lack of that, I always disliked those timeouts, so I think this change is great. |
|
This isn't the timeout you want to remove - This controls how long we wait when we already have multiple RPCs in flight, so removing it means we'll wait as long as it takes for them to complete. You want to remove SendNextTimeout to avoid having multiple RPCs in flight in the first place. Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
|
@bdarnell I also chatted with @tamird because I thought the PR description suggested that's what he wanted to do, but it seems intentional that he's only removing this smaller bit now (which to me seems also reasonable, and perhaps less contentious). Perhaps the PR message should be clarified though. |
|
Ah. Well in that case I disagree with this change. The reliance on a single leader means that we may not need to send RPCs to multiple replicas, but having sent them, they may be slow and we should use timeouts when waiting on secondary RPC attempts. |
pkg/kv/dist_sender.go
Outdated
| } | ||
| } else if pendingCall.Reply.Error == nil { | ||
| return pendingCall.Reply, nil | ||
| for ; pending > 0; pending-- { |
There was a problem hiding this comment.
There's actually no point in waiting for these pending RPCs unless haveCommit is true. Let's change this line to:
for ; haveCommit && pending > 0; pending-- {
|
I think @bdarnell has a point: removing this timeout entirely means a client can become permanently stuck if a range is jammed and not processing Raft commands. I think it might make more sense to increase the value of the timeout instead (perhaps to We can rely on the gRPC heartbeat timeout to cover the case of a connection which has become too unhealthy to serve RPCs. That's a Also, @tamird, please note my comment around |
|
This could only break multi-node benchmarks, right? If this is somehow affecting single-node benchmarks I think there is a bug somewhere. |
|
Revised this to increase the timeout instead of removing it, PTAL |
|
@petermattis indeed, only affects multi-node benchmarks. |
tamird
changed the title
|
@tamird The multi-node |
|
Yep, I'm proceeding that way.
…On Wed, May 24, 2017 at 2:02 PM, Peter Mattis ***@***.***> wrote:
@tamird <https://github.com/tamird> The multi-node sql benchmarks are
somewhat uninteresting in that the nodes are all running on the same
machine. That's not to dissuade this PR, but to help you unblock the
benchmark storage work. I'd be fine in the short term with only recording
the single-node sql benchmarks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16088 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPLVVb7OiFo-steo5srDE6t5ANP3Rks5r9HCxgaJpZM4NkKlL>
.
|
|
lgtm |
|
Reviewed 6 of 7 files at r3, 1 of 2 files at r4, 1 of 1 files at r5. Comments from Reviewable |
This timeout is believed to protect against hanging RPCs. Unfortunately whenever such an abandonment takes place, an ambiguous result error is returned to the client, which can be difficult to deal with. This greatly increases the timeout in the hope of reducing these errors at the expense of tail latencies. The upshot is that if such hanging RPCs are common, we'll be able to track them down.
No description provided.