client: addrConn NewStream and health check cleanup#2848
client: addrConn NewStream and health check cleanup#2848menghanl merged 8 commits intogrpc:masterfrom
Conversation
menghanl
commented
May 31, 2019
- Make newStream() take string (for method) so it can be used for out-of-band load reporting (which will also be a stream on an addrConn)
- move healthcheck (streaming) related functions into a function
- make ac.newClientStream not a method of addrConn
- so it's (should be) easier to merge with cc.newStream()
| } | ||
| } | ||
| c.maxReceiveMessageSize = getMaxSize(nil, c.maxReceiveMessageSize, defaultClientMaxReceiveMessageSize) | ||
| c.maxSendMessageSize = getMaxSize(nil, c.maxSendMessageSize, defaultServerMaxSendMessageSize) |
There was a problem hiding this comment.
why change the position of this block?
There was a problem hiding this comment.
This is more similar to clientConn.NewStream().
| if ac.transport != currentTr { | ||
| return nil, status.Error(codes.Canceled, "the provided transport is no longer valid to use") | ||
| } | ||
| return newNonRetryClientStream(ctx, &StreamDesc{ServerStreams: true}, method, currentTr, ac) |
There was a problem hiding this comment.
newNonRetryClientStream holds the ac.mu lock the whole time. Is this necessary?
| return nil, status.Error(codes.Canceled, "the provided transport is no longer valid to use") | ||
| } | ||
| // transition to CONNECTING state when an attempt starts | ||
| if ac.state != connectivity.Connecting { |
There was a problem hiding this comment.
deleting this state transition will lead to invalid state transition such as IDLE -> READY, TRANSIENT FAILURE -> READY.
There was a problem hiding this comment.
Changing connectivity state when creating a stream on the addrConn in general doesn't sound right.
Also, this is to create a general purpose stream on the addrConn (as opposite to a health checking stream), so changing connectivity state is just not going to work.
TransientFailure<->Ready is already happening today with the health check stream.
This function is called only when connection is created, so the state is not IDLE.
There was a problem hiding this comment.
TransientFailure<->Readyis already happening today with the health check stream.
This is not true, because newStream() will set ac's state to connecting.
The fix is to manually set state to connecting before calling newStream(), in the health checking goroutine.
There was a problem hiding this comment.
No, the "invalid" transition still happens when the health checking stream is good, but server reports unhealthy. But we still need the manual transition to connecting when stream is broken (this behavior is defined in the spec).
9f7e49f to
e511624
Compare
dfawley
left a comment
There was a problem hiding this comment.
Looks great, thanks for the cleanups. Just a couple very small things to consider.
health/client_test.go
Outdated
| defer func() { backoffFunc = oldBackoffFunc }() | ||
|
|
||
| clientHealthCheck(context.Background(), newStream, func(_ bool) {}, "test") | ||
| clientHealthCheck(context.Background(), newStream, func(_ connectivity.State) {}, "test") |
There was a problem hiding this comment.
Done.
Why doesn't gofmt fix this?
There was a problem hiding this comment.
Because you haven't made it do that yet! 😁
internal/internal.go
Outdated
| // | ||
| // The health checking protocol is defined at: | ||
| // https://github.com/grpc/grpc/blob/master/doc/health-checking.md | ||
| type HealthChecker func(ctx context.Context, newStream func(string) (interface{}, error), reportHealth func(connectivity.State), serviceName string) error |
There was a problem hiding this comment.
Should reportHealth be renamed to setSubchannelState or something like that?
e511624 to
d1a2f44
Compare