You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/03/19 09:21:16 UTC

[GitHub] [zookeeper] symat opened a new pull request #1288: ZOOKEEPER-3758: Leader reachability check fails with single address

symat opened a new pull request #1288: ZOOKEEPER-3758: Leader reachability check fails with single address
URL: https://github.com/apache/zookeeper/pull/1288
 
 
   Since ZooKeeper 3.6.0 we can specify multiple addresses for each ZooKeeper server instance
   (this can increase availability when multiple physical network interfaces can be used parallel
   in the cluster). ZooKeeper will perform ICMP ECHO requests or try to establish a TCP connection
   on port 7 (Echo) of the destination host in order to find the reachable addresses. This should
   happen only if the user provide multiple addresses in the configuration, in a single addess is
   used then ZooKeeper shouldn’t send any ICMP requests.
   
   This works as we expected for the leader election connections, but in this Jira issue we found
   a bug when the reachability check was performed even with a single address when the Follower
   tries to connect to the newly elected Leader.
   
   The fix follows the same approach we discussed for the election protocol before (see
   ZOOKEEPER-3698). We avoid the reachability check for single addresses. Also when we have
   multiple addresses and none of them can be reached, we still start to connect to all addresses
   in parallel.

----------------------------------------------------------------
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

[GitHub] [zookeeper] symat commented on issue #1288: ZOOKEEPER-3758: Leader reachability check fails with single address

Posted by GitBox <gi...@apache.org>.
symat commented on issue #1288: ZOOKEEPER-3758: Leader reachability check fails with single address
URL: https://github.com/apache/zookeeper/pull/1288#issuecomment-601214350
 
 
   @eolivelli  @nkalmar  thanks for checking this so quickly!

----------------------------------------------------------------
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

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

Posted by 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

[GitHub] [zookeeper] asfgit closed pull request #1288: ZOOKEEPER-3758: Leader reachability check fails with single address

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1288: ZOOKEEPER-3758: Leader reachability check fails with single address
URL: https://github.com/apache/zookeeper/pull/1288
 
 
   

----------------------------------------------------------------
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

[GitHub] [zookeeper] nkalmar commented on issue #1288: ZOOKEEPER-3758: Leader reachability check fails with single address

Posted by GitBox <gi...@apache.org>.
nkalmar commented on issue #1288: ZOOKEEPER-3758: Leader reachability check fails with single address
URL: https://github.com/apache/zookeeper/pull/1288#issuecomment-601268743
 
 
   pushed to master and branch-3.6, thanks @symat 

----------------------------------------------------------------
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

[GitHub] [zookeeper] nkalmar commented on issue #1288: ZOOKEEPER-3758: Leader reachability check fails with single address

Posted by GitBox <gi...@apache.org>.
nkalmar commented on issue #1288: ZOOKEEPER-3758: Leader reachability check fails with single address
URL: https://github.com/apache/zookeeper/pull/1288#issuecomment-601147072
 
 
   I messed up my python env, and the script is not running, I'll try to fix it. I can merge after.

----------------------------------------------------------------
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

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

Posted by GitBox <gi...@apache.org>.
eolivelli 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_r394934877
 
 

 ##########
 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:
   Isn't this disabled by default?

----------------------------------------------------------------
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

[GitHub] [zookeeper] eolivelli commented on issue #1288: ZOOKEEPER-3758: Leader reachability check fails with single address

Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #1288: ZOOKEEPER-3758: Leader reachability check fails with single address
URL: https://github.com/apache/zookeeper/pull/1288#issuecomment-601133104
 
 
   @nkalmar this is a good candidate for 3.6.1
   
   please take a look

----------------------------------------------------------------
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

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

Posted by 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