Set peer before sending request #1423
Conversation
|
It's worth noting that this may impact the retry code that @dfawley is working on (and I'm running into a similar problem in C-core). The peer that we are talking to when we send the first message may not be the one that ultimately successfully handles the request, so if we set the peer when we send the first message and then retry on a different backend later, the peer information we originally set may be incorrect. We probably need to discuss the right semantics for returning peer information in the context of retries (and the hedging case is even more unclear, since we may be talking to multiple backends simultaneously). |
|
Retries are the kind of scenario I had in mind when I said in the original issue that maybe the peer could be set twice. And yeah, hedging is even trickier. Personally, I'd be OK if initially gRPC just tracked the last peer that succeeded/failed. |
|
@markdroth interesting, thanks for calling this to my attention. I think this change is OK for us as long as nobody accesses the peer while the RPC is happening. Our API is synchronous, so this would be unexpected. For retries, it would end up being set to the last attempt, which is presumably what we want. For hedging, we will have to move things around a bit, but this won't make things much worse than they already are in that regard. |
test/end2end_test.go
Outdated
| } | ||
| break | ||
| } | ||
| time.Sleep(10 * time.Millisecond) |
There was a problem hiding this comment.
It's not needed, I'll remove it.
There was a problem hiding this comment.
I remembered why you need it, you need to give it sufficient time to reach the block of code that sets the peer.
There was a problem hiding this comment.
How about we relied on something else to cause the RPC to fail, like may be trying to send a message size bigger than the max allowed message size?
This call option can be used to set a max message size allowed for a call. And the test can then deliberately try and send a message bigger than what you set.
There was a problem hiding this comment.
I've made the requested change, thanks!
If a connection to the server exists but a request errors out due to context canceled or deadline exceeded, the peer information is not set. We can move setting the peer out of the recvResponse and before the sendRequest to get peer information for some failed RPCs. This addresses #1417
@MakMukhi @therc