Client load report#1200
Conversation
Before moving grpclb into grpc package. This reverts commit 3c582a8.
0f09abc to
18c603c
Compare
436fbcb to
f41c688
Compare
| } | ||
| if put != nil { | ||
| put() | ||
| put = nil |
There was a problem hiding this comment.
Why nil out put? Unless I'm reading it wrong, it's local to this loop, and we either return or continue after this.
(Same below.)
| stream, err = sendRequest(ctx, cc.dopts, cc.dopts.cp, callHdr, t, args, topts) | ||
| stream, err = t.NewStream(ctx, callHdr) | ||
| if err != nil { | ||
| if _, ok := err.(transport.ConnectionError); ok && put != nil { |
There was a problem hiding this comment.
Maybe combine the ifs here:
if put != nil {
if _, ok := err.(transport.ConnectionError); ok {
updateRPCStats()
}
put()
}?
| put() | ||
| put = nil | ||
| } | ||
| if _, ok := err.(transport.ConnectionError); ok || err == transport.ErrStreamDrain { |
There was a problem hiding this comment.
How about:
if _, ok := err.(transport.ConnectionError); (ok || err == transport.ErrStreamDrain) && !c.failFast {
continue
}
return toRPCErr(err)| } | ||
| return toRPCErr(err) | ||
| } | ||
| updateRPCStatsInContext(ctx, rpcStats{ |
There was a problem hiding this comment.
Why is this update here? Presumably nobody will care about the stats until put is called. (It's also not guarded by a put != nil)
| } | ||
|
|
||
| func updateRPCStatsInContext(ctx context.Context, s rpcStats) { | ||
| ss, ok := rpcStatsFromContext(ctx) |
There was a problem hiding this comment.
if ss, ok := rpcStatsFromContext(ctx); ok {
*ss = s
}| b.clientStats = lbpb.ClientStats{} // Clear the stats. | ||
| b.mu.Unlock() | ||
| t := time.Now() | ||
| stats.Timestamp = &lbpb.Timestamp{ |
There was a problem hiding this comment.
FYI: there's a helper in ptypes for setting Timestamp messages. It would probably be best to use that.
There was a problem hiding this comment.
The timestamp here is a copy of the ptypes Timestamp...
| return | ||
| } | ||
| b.mu.Lock() | ||
| stats := b.clientStats |
There was a problem hiding this comment.
This makes an unnecessary copy. You could instead use b.clientStats directly and clear it after calling Send.
There was a problem hiding this comment.
b.clientStats is protected by the mutex. I don't want to put the rpc call between the lock/unlock.
| seq := b.seq | ||
|
|
||
| defer func() { | ||
| if err == nil { |
There was a problem hiding this comment.
Preferred go-ism:
if err != nil {
return
}And outdent the following code.
| if !opts.BlockingWait { | ||
| b.next = next | ||
| if a.dropForLoadBalancing { | ||
| b.clientStats.NumCallsFinished++ |
There was a problem hiding this comment.
Optional: in the defer, if err != nil, do this b.clientStats.NumCallsFinished++.
There was a problem hiding this comment.
In that case, these two values will not be protected by the same pair of lock/unlock.
| return nil | ||
| } | ||
|
|
||
| type rpcStats struct { |
There was a problem hiding this comment.
How about rpcInfo instead? Stats makes me think there are metrics inside. Combine "stats" with bytesSent/Received, and I expect those to be ints instead of bools.
This PR implements client load report for grpclb.