-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
etcdserver: not reuse connections for peer transport #3796
Conversation
Why does it work previously? The peer listener has always been time out one, no? |
The peer transporter was a timeout transport, which sets maxIdleConnections as -1 for itself.
yes |
then why do not create a timeout transport instead? also can you please fix all the transport that works with peer url? |
it doesn't need read timeout and write timeout in connection because it is not a stream. I could create a timeout transport with no read/write timeout though. |
Why only stream can have read/write timeout? |
The read/write timeout has the assumption that following reads/writes will happen soon. |
I do not think you are solving the root problem. The root reason is that the underlying listener for peer http is a timeout listener. It expects the transport to send/recv fast and never reuse the connection. We built the other pair of transport for it already. We really should just use it instead of creating something new with some new reasons. Please use the correct pairs of transport/listener. Thanks. |
This pairs with remote timeout listeners. etcd uses timeout listener, and times out the accepted connections if there is no activity. So the idle connections may time out easily. Becaus timeout transport doesn't reuse connections, it prevents using timeouted connection. This fixes the problem that etcd fail to get version of peers.
f293849
to
0eee88a
Compare
ok. They are used as a pair. So I changes to use timeoutTransport, and fixes other transport that has the same problem. PTAL. |
Per xiang's suggestion, we want to avoid the same bug in the future. New commit moves the code to create listener and transport for peer communication |
@@ -97,6 +99,28 @@ type Transporter interface { | |||
Stop() | |||
} | |||
|
|||
// NewPeerListener returns a listener for raft message transfer between peers. | |||
// It uses timeout listener to identify broken streams promptly. | |||
func NewPeerListener(u url.URL, tlsInfo transport.TLSInfo) (net.Listener, error) { |
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.
Can we call this NewRaftListener?
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.
sure.
46b251d
to
62d8aef
Compare
I changed the function name to We thought about the idea that makes the new function named PTAL. |
@xiang90 points out that |
It uses roundTripper instead of Transport because roundTripper is sufficient for its requirements.
62d8aef
to
abd7050
Compare
@xiang90 Changed it to |
This moves the code to create listener and roundTripper for raft communication to the same place, and use explicit functions to build them. This prevents possible development errors in the future.
abd7050
to
4ccbcb9
Compare
Based on discussion OOL, moved new functions into util.go and add comments about 0 read/write timeout. |
LGTM |
etcdserver: not reuse connections for peer transport
etcd uses timeout listener, and times out the accepted connections
if there is no activity. If we use idle connections in
transport, the connection may have timed out, and return error when
doing any request. So we do not reuse connections.
This fixes the problem that etcd fail to get version of peers.
fixes #3723