Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZOOKEEPER-3758: Leader reachability check fails with single address #1288

Closed
wants to merge 1 commit into from
Closed

ZOOKEEPER-3758: Leader reachability check fails with single address #1288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,9 @@ protected void connectToLeader(MultipleAddresses multiAddr, String hostname) thr
this.leaderAddr = multiAddr;
Set<InetSocketAddress> addresses;
if (self.isMultiAddressReachabilityCheckEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this disabled by default?

Copy link
Contributor Author

@symat symat Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/MultipleAddresses.java

Lines 151 to 158 in a5a4743

public InetSocketAddress getReachableOrOne() {
InetSocketAddress address;
// if there is only a single address provided then we don't do any reachability check
if (addresses.size() == 1) {
return getOne();
}

basically the same logic was missing from the MultipleAddresses.getAllReachableAddresses method.

I should have thinking of it, but forget when I fixed ZOOKEEPER-3698.

addresses = multiAddr.getAllReachableAddresses();
// even if none of the addresses are reachable, we want to try to establish connection
// see ZOOKEEPER-3758
addresses = multiAddr.getAllReachableAddressesOrAll();
} else {
addresses = multiAddr.getAllAddresses();
}
Expand Down
18 changes: 18 additions & 0 deletions zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/MultipleAddresses.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,24 @@ public Set<InetSocketAddress> getAllReachableAddresses() {
.collect(Collectors.toSet());
}

/**
* Returns a set of all reachable addresses. If none is reachable than returns all addresses.
*
* @return all reachable addresses, or all addresses if none is reachable.
*/
public Set<InetSocketAddress> getAllReachableAddressesOrAll() {
// if there is only a single address provided then we don't need to do any reachability check
if (addresses.size() == 1) {
return getAllAddresses();
}

Set<InetSocketAddress> allReachable = getAllReachableAddresses();
if (allReachable.isEmpty()) {
return getAllAddresses();
}
return allReachable;
}

/**
* Returns a reachable address or an arbitrary one, if none is reachable. It throws an exception
* if there are no addresses registered. The function is nondeterministic in the sense that the
Expand Down
26 changes: 26 additions & 0 deletions zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/MultipleAddressesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,32 @@ public void testGetAllReachableAddresses() throws Exception {
Assert.assertEquals(reachableHosts, multipleAddresses.getAllReachableAddresses());
}

@Test
public void testGetAllReachableAddressesOrAllWhenSomeReachable() throws Exception {
InetSocketAddress reachableHost1 = new InetSocketAddress("127.0.0.1", 1234);
InetSocketAddress reachableHost2 = new InetSocketAddress("127.0.0.1", 2345);
InetSocketAddress unreachableHost1 = new InetSocketAddress("unreachable1.address.zookeeper.apache.com", 1234);
InetSocketAddress unreachableHost2 = new InetSocketAddress("unreachable2.address.zookeeper.apache.com", 1234);

MultipleAddresses multipleAddresses = new MultipleAddresses(
Arrays.asList(unreachableHost1, unreachableHost2, reachableHost1, reachableHost2));

Set<InetSocketAddress> reachableHosts = new HashSet<>(Arrays.asList(reachableHost1, reachableHost2));
Assert.assertEquals(reachableHosts, multipleAddresses.getAllReachableAddressesOrAll());
}

@Test
public void testGetAllReachableAddressesOrAllWhenNoneReachable() throws Exception {
InetSocketAddress unreachableHost1 = new InetSocketAddress("unreachable1.address.zookeeper.apache.com", 1234);
InetSocketAddress unreachableHost2 = new InetSocketAddress("unreachable2.address.zookeeper.apache.com", 1234);
InetSocketAddress unreachableHost3 = new InetSocketAddress("unreachable3.address.zookeeper.apache.com", 1234);
List<InetSocketAddress> allUnreachableAddresses = Arrays.asList(unreachableHost1, unreachableHost2, unreachableHost3);

MultipleAddresses multipleAddresses = new MultipleAddresses(allUnreachableAddresses);

Assert.assertEquals(new HashSet<>(allUnreachableAddresses), multipleAddresses.getAllReachableAddressesOrAll());
}

@Test
public void testEquals() {
List<InetSocketAddress> addresses = getAddressList();
Expand Down