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.
yichengq
force-pushed
the
fix-get-version
branch
from
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.
Can we call this NewRaftListener?
yichengq
force-pushed
the
fix-get-version
branch
2 times, most recently
from
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.
yichengq
force-pushed
the
fix-get-version
branch
from
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.
yichengq
force-pushed
the
fix-get-version
branch
from
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