-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not create new addrConn when connection error happens #1369
Conversation
24a95a7
to
9a81212
Compare
6961ab6
to
df9862c
Compare
clientconn.go
Outdated
select { | ||
case <-t.Error(): | ||
ac.cc.resetAddrConn(ac.addr, false, errNetworkIO) | ||
if err := ac.resetTransport(false); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor?
var drain bool
select {
case <-t.Error():
default:
drain = true
}
if err := ac.resetTransport(drain); err != nil {
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple comment nits.
clientconn.go
Outdated
// 2) goaway was received, a new ac will replace the old ac. | ||
// The old ac should be deleted from cc.conns, but the | ||
// underlying transport should drain rather than close. | ||
// a buggy Balancer notifies duplicated Addresses; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"a buggy Balancer that reports duplicate Addresses."
clientconn.go
Outdated
@@ -949,28 +949,22 @@ func (ac *addrConn) transportMonitor() { | |||
ac.adjustParams(t.GetGoAwayReason()) | |||
// If GoAway happens without any network I/O error, the underlying transport | |||
// will be gracefully closed, and a new transport will be created. | |||
// (the transport will be closed when all the pending RPCs finished or failed.). | |||
// (the transport will be closed when all the pending RPCs finished or failed.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capital "T" at the start of the sentence.
7f43dea
to
f305355
Compare
f305355
to
b67bdb1
Compare
Fix etcd-io#8329. Different behavior from grpc/grpc-go#1369, in grpc-go transportMonitor. Signed-off-by: Gyu-Ho Lee <[email protected]>
Fix etcd-io#8329. Different behavior from grpc/grpc-go#1369, in grpc-go transportMonitor. Signed-off-by: Gyu-Ho Lee <[email protected]>
Just reset the underlying transport instead.