client: move everything to rpcSender #2351
Conversation
| @@ -77,12 +79,20 @@ func (ctx *Context) InitDefaults() { | |||
| ctx.User = defaultUser | |||
| } | |||
|
|
|||
| // RequestScheme returns "http" or "https" based on the value of Insecure. | |||
| func (ctx *Context) RequestScheme() string { | |||
| // RPCRequestScheme returns "http" or "https" based on the value of Insecure. | |||
|
What's the motivation for this change? Is there an issue or was it just making the codebase cleaner? |
|
Performance and cleanup.
|
| } | ||
| func (s *rpcSender) Send(_ context.Context, call proto.Call) { | ||
| // Wait for it. | ||
| <-s.client.Healthy() |
There was a problem hiding this comment.
No retry-loop anymore? Perhaps this is explained elsewhere in the change.
There was a problem hiding this comment.
Had to remove this because the assumption that all RPCs are retryable made it impossible to test certain failures. I'll put it back and remove the offending tests if this gets @mberhault's approval.
There was a problem hiding this comment.
Added the retry loop back
|
LGTM modulo the removal of the retry-loop in rpc sending. And @mberhault should definitely weigh-in as he has more familiarity with this code. |
| client := roachrpc.NewClient(addr, ctx) | ||
| return &Sender{ | ||
|
|
||
| ctx := rpc.NewContext(context, hlc.NewClock(hlc.UnixNano), stop.NewStopper()) |
There was a problem hiding this comment.
this is probably wrong in that it's getting a new stopper, but i'm not sure what else to do. suggestions?
There was a problem hiding this comment.
Why does it need a stopper now when it didn't before? When is this stopper stopped? The most obvious solution would be to pass a stopper in (modifying NewSenderFunc and on up the stack); is there any reason that wouldn't work?
There was a problem hiding this comment.
It wasn't being used before. When is the stopper stopped is a good question; whenever the client is no longer needed?
We could plumb it through, I'm just not sure what would be at the top. Probably tests and cli commands.
There was a problem hiding this comment.
Is this necessary because tests previously used the http sender and are now using the rpc sender? If yes, passing the stopper to NewSenderFunc sounds correct.
|
LGTM. Which tests fail with the retry loop? |
|
@bdarnell it was TestKVDBInternalMethods but I fixed it by limiting the number of retries. |
| @@ -140,8 +138,8 @@ func TestClientRetryNonTxn(t *testing.T) { | |||
| // intent to be pushed and once with priorities which will not. | |||
| for i, test := range testCases { | |||
| key := proto.Key(fmt.Sprintf("key-%d", i)) | |||
| txnPri := 1 | |||
| clientPri := 1 | |||
| var txnPri int32 = 1 | |||
There was a problem hiding this comment.
I prefer the syntax txnPri := int32(1) if you need an explicit type for txnPri. Doesn't particularly matter, though.
|
LGTM. |
|
OK I plumbed the stopper through. Can I get a final review? I'll squash the last commit into the main one and merge after an LGTM. |
The client code wants to know which request failed, but with the `(response, error)` pattern no reply is returned on error, so no reply header can be used to find out the location of the error. This needs further fixing, see #1891.
replaces everything with rpcSender
|
@tschottdorf I had to steal your client fix. |
client: move everything to rpcSender
|
Sure. What made it necessary? |
|
@tschottdorf see #2370 |
I was possibly heavy handed with this. Nits welcome.
Fixes #2271.