[GitHub] [zookeeper] symat commented on a change in pull request #1288: ZOOKEEPER-3758: Leader reachability check fails with single address

[GitBox <gi...@apache.org>]

symat commented on a change in pull request #1288: ZOOKEEPER-3758: Leader reachability check fails with single address
URL: https://github.com/apache/zookeeper/pull/1288#discussion_r395072587
 
 

 ##########
 File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java
 ##########
 @@ -303,7 +303,9 @@ protected void connectToLeader(MultipleAddresses multiAddr, String hostname) thr
         this.leaderAddr = multiAddr;
         Set<InetSocketAddress> addresses;
         if (self.isMultiAddressReachabilityCheckEnabled()) {
 
 Review comment:
   no, unfortunately not. There are two separate parameters:
   - multiAddress.enabled : this is for enabling/disabling the whole multiaddress feature, this is set to false to default
   - multiAddress.reachabilityCheckEnabled : this one can be used to disable the ICMP messages when multiple addresses are used. But this is enabled by default, as disabling it actually makes the multiAddress recovery unreliable (as ZK will not really be able to select among the addresses)
   
   There is already a logic in the MultiAddress.getReachableOrOne() method which will skip the ICMP check if there is only a single address is provided:
   https://github.com/apache/zookeeper/blob/a5a4743733b8939464af82c1ee68a593fadbe362/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/MultipleAddresses.java#L151-L158
   
   basically the same logic was missing from the {{MultipleAddresses.getAllReachableAddresses}} method.
   
   I should have thinking of it, but forget when I fixed ZOOKEEPER-3698.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services