Client load report #1200
Client load report #1200
Conversation
Before moving grpclb into grpc package. This reverts commit 3c582a8.
menghanl
force-pushed
the
client_load_report
branch
from
0f09abc
to
18c603c
Compare
menghanl
force-pushed
the
client_load_report
branch
from
436fbcb
to
f41c688
Compare
call.go
Outdated
| } | ||
| 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.)
call.go
Outdated
| 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()
}?
call.go
Outdated
| 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)
call.go
Outdated
| @@ -277,11 +302,19 @@ func invoke(ctx context.Context, method string, args, reply interface{}, cc *Cli | |||
| } | |||
| 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)
rpc_util.go
Outdated
| } | ||
|
|
||
| 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.
grpclb.go
Outdated
| 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.
| @@ -556,6 +618,13 @@ func (b *balancer) Get(ctx context.Context, opts BalancerGetOptions) (addr Addre | |||
| } | |||
| 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.
rpc_util.go
Outdated
| @@ -345,6 +345,30 @@ func recv(p *parser, c Codec, s *transport.Stream, dc Decompressor, m interface{ | |||
| 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.