handle TCP transports properly #201
Conversation
There was a problem hiding this comment.
Not having made a super thorough look into memberlist my thinking is that conceptually the transport should handle any underlying protocol specific behavior and this shouldn't have to be cared about int state.go.
I am not sure how we could accomplish this but it just seems odd that protocol specifics are in the state code and that maybe the Transport interface should be updated more extensively to provide an interface where more of the protocol specifics can be left to them.
|
@mkeeler thanks for looking at it! I am trying to imagine what your transport would look like and I am not sure what you have in mind. I don't think the transport should handle these case, since they are not transport internal, but memberlist internal. This code changes the flow in The code in |
transport.go
Outdated
| @@ -62,4 +69,6 @@ type Transport interface { | |||
| // Shutdown is called when memberlist is shutting down; this gives the | |||
| // transport a chance to clean up any listeners. | |||
| Shutdown() error | |||
|
|
|||
| ProtocolType() ProtocolType | |||
There was a problem hiding this comment.
Note this is a breaking change to the exported interface. Any custom implementations of the interface will no longer implement this interface.
An alternative would be making a new interface like:
type TypedTransport interface {
Transport
ProtocolType() ProtocolType
}
Then you just have to do optional type assertions to see if the transport implements the extra method. I recently did this in memberlist when i had to change Broadcast: https://github.com/hashicorp/memberlist/blob/master/queue.go#L128
state.go
Outdated
| } else { | ||
| close(fallbackCh) | ||
| } | ||
| if m.transport.ProtocolType() == ProtocolTypeUDP { |
There was a problem hiding this comment.
Ugh. So my earlier suggestion about piggybacking on the error in WriteTo falls apart at this line.
There was a problem hiding this comment.
Yeah, but my net.OpError checking as well. This is why I took that out again. It is just an optimization. Since TCP failed before, it will likely fail again. I am fine with that, considering how broken it is atm.
| } | ||
| return false | ||
| } | ||
|
|
||
| // probeNode handles a single round of failure checking on a node. | ||
| func (m *Memberlist) probeNode(node *nodeState) { |
There was a problem hiding this comment.
maybe also worth adding a test that forces the mock transport to fail in the manner of a tcp failure so you can assert this entire method functions as you expect
There was a problem hiding this comment.
good point. I tried to do that, but turns out it is not that easy. :( ended up not getting it in.
There was a problem hiding this comment.
I did however ran many many tests with a consul cluster setup that had network areas.
The underlying issue is that UDP and TCP are different. And since any transport is allowed, memberlist must account for both cases. For example with TCP a ping fails while sending, while with UDP there is no response.
If there would be a way to detect if a transport is TCP or UDP I would prefer that over implementing
ProtocolType().I checked the other occurrences of
encodeAndSendMsgand nothing else should need special handling.