avoid repeat connections in pings every round #21812
Conversation
There was a problem hiding this comment.
Thx @chengpohi ! I left some comments. It would also be great to have a test that the unicast host provider is only called once per pinging round. Also it would be good to have a test that pinging picks up on nodes that it discovered - i.e., if you A & C ping B , A starts pinging C as well (it got it through B). See UnicastZenPingTests for examples and where this should go.
| final UnicastPingRequest pingRequest = new UnicastPingRequest(); | ||
| pingRequest.id = sendPingsHandler.id(); | ||
| pingRequest.timeout = timeout; | ||
| DiscoveryNodes discoNodes = contextProvider.nodes(); | ||
|
|
||
| pingRequest.pingResponse = createPingResponse(discoNodes); | ||
|
|
||
| HashSet<DiscoveryNode> nodesToPingSet = new HashSet<>(); | ||
| for (PingResponse temporalResponse : temporalResponses) { |
There was a problem hiding this comment.
this part has to stay here - we want to extend our pinging as we learn of new nodes
There was a problem hiding this comment.
okay, I mv back this. Thanks
| @@ -292,6 +292,7 @@ public void clearTemporalResponses() { | |||
| */ | |||
| @Override | |||
| public void ping(final PingListener listener, final TimeValue duration) { | |||
| final List<DiscoveryNode> sortedNodesToPing = buildNodesToPing(); | |||
There was a problem hiding this comment.
can we unify the resolvedDiscoveryNodes logic with the buildNodesToPing ? They are similar in the sense that they don't change during a ping cycle.
There was a problem hiding this comment.
Hi @bleskes, I have updated this base on your suggestion, Could you review again? I am using Tuple for keeping resolvedDiscoveryNodes and nodesToPingSet, since we need to sort nodesToPingSet to elect the master node.
…h sendPings, and add unihostprovider test, pick up discovered node test
| @@ -518,6 +503,33 @@ public void run() { | |||
| } | |||
| } | |||
|
|
|||
| private Tuple<List<DiscoveryNode>, HashSet<DiscoveryNode>> buildNodesToPing() { | |||
There was a problem hiding this comment.
we typically don't like using tuple return values. In this case I think we can just return a HashSet.
|
|
||
| final List<DiscoveryNode> resolvedDiscoveryNodes; | ||
| try { | ||
| resolvedDiscoveryNodes = resolveDiscoveryNodes( |
There was a problem hiding this comment.
You can add the return value of resolveDiscoveryNodes to the HashSet being built
There was a problem hiding this comment.
Hi @bleskes , if we add resolveDiscoveryNodes, we can't guarantee configured hosts first ping, Does it make sense?
| verify(unicastHostsProviderA, times(1)).buildDynamicNodes(); | ||
| } | ||
|
|
||
| public void testShouldPingDiscoveredNodes() throws IOException, InterruptedException { |
There was a problem hiding this comment.
can we call this testDiscoveryOfNoneConfiguredNodes and add a comment: test that nodes discover each other if they ping a common host ?
This doesn't test what I intended - i.e., test that a node pings nodes that have pinged it while it was pinging. That is however non trivial to test and I don't want to delay your change for setting this test up.
There was a problem hiding this comment.
Thanks your feedback, I will take time to see how to this test, if I have any progress on this, I will create a new PR for this. :)
clintongormley
added
:Distributed Coordination/Discovery-Plugins
…dNodes to testDiscoveryOfNoneConfiguredNodes
|
Thanks for picking this up @chengpohi but this has now been superseded by #22277. |
This PR is trying to fix: #21739 by Option 1.
@bleskes Could you review this?