ZOOKEEPER-3100: ZooKeeper client times out due to random choice of resolved addresses

[anmolnar]

I ended up removing the random shuffle when resolving DNS names. This way we will honor JVM setting java.net.preferIPv6Addresses, because the order of returned address are already influenced by this setting. If multiple addresses are returned for the same host in the same address type (v4/v6), we'll try each in every round, because DNS already randomize the addresses within the same class.

[phunt]

This code has been this way as long as I can remember - it seems like it could have a significant impact to existing functionality/expectations. eg how do we know that every dns provider will behave as you expect. I would imagine something like this behind a config option, release notes and documentation would be less risky.

[anmolnar]

This code has been this way as long as I can remember - it seems like it could have a significant impact to existing functionality/expectations. eg how do we know that every dns provider will behave as you expect. I would imagine something like this behind a config option, release notes and documentation would be less risky.

That's good point. Currently there're two issues with this code.

1. ZooKeeper client doesn't care about java.net.preferIPv6Addresses setting and picks randomly from the resolved IP addresses regardless of IP version.
2. Choosing completely randomly from the returned addresses also ignores DNS round robin implementation if present.

Now both issues are covered by the patch, but if we want to make this optional, which part shall we introduce a config setting?

1. Introduce zookeeper.groupDnsResponse (false by default) and group DNS response by address type and choose randomly from the preferred group.
2. Introduce zookeeper.shuffleDnsResponse (true by default) and keep the original behavior completely if set to true.
3. Combine the previous two options and introduce both settings to control the two different behaviors individually.

[kezhuw]

Can we also do the same for addresses that each of the hosts resolves to, so that backoffs are only applied after connection to each address is attempted once and every address is connected to once using round-robin rather than random selection? This will avoid delays in cases where at least one address can be connected to. -- quoted from jira issue

I think we can iterate every resolved addresses in jdk given order so we obey both java.net.preferIPv4Stack and java.net.preferIPv6Addresses and solve ZOOKEEPER-3100.

-1 to new properties. We probably will run into interference or inconsistence with system properties just like ZOOKEEPER-4955.

See:

• https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/net/spi/InetAddressResolver.java#L89

[anmolnar]

I think we can iterate every resolved addresses in jdk given order so we obey both java.net.preferIPv4Stack and java.net.preferIPv6Addresses and solve [ZOOKEEPER-3100](https://issues.apache.org/jira/browse/ZOOKEEPER-3100

I don't think we should iterate over all addresses. Thought about this, but I believe it's better to pick one address from DNS according to java system properties and if it fails, immediately move to next server. If a single server fails, it most likely will fail on all addresses, because it's down. In the next iteration when we come back to this server again, DNS will give another IP address if available.

[anmolnar]

-1 to new properties. We probably will run into interference or inconsistence with system properties just like ZOOKEEPER-4955.

See:

• https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/net/spi/InetAddressResolver.java#L89

Are u saying that patch is fine as it is now?

[kezhuw]

If a single server fails, it most likely will fail on all addresses, because it's down.

Good point!

+1 to this pr or zookeeper.shuffleDnsResponse, but not zookeeper.groupDnsResponse which is overlapping with jdk properties.

[kezhuw]

how do we know that every dns provider will behave as you expect. I would imagine something like this behind a config option, release notes and documentation would be less risky.

Introduce zookeeper.shuffleDnsResponse (true by default) and keep the original behavior completely if set to true.

If we are going to introduce option for this, I would prefer to turn shuffle disabled by default. This way zookeeper probably will function consistent in dns with other softwares on the same host.

[anmolnar]

So, number 2) is ...

2. Introduce zookeeper.shuffleDnsResponse (true by default) and keep the original behavior completely if set to true.

which is going to be a feature flag which toggles the old behavior. By default, however, should be turned off meaning using the new behavior implemented in this patch.

if (shuffleDnsResponse) {
    Collections.shuffle(resolvedAddresses);
}

@phunt wdyt?

[anmolnar]

@kezhuw @phunt Added feature flag and docs. PTAL.

[anmolnar]

Looks like I've found a problem: InetAddress.getAllByName() doesn't really returns the IP addresses in the right order according to java.net.preferIPv6Addresses setting. We might need to implement it ourselves.

[anmolnar]

Looks like I've found a problem: InetAddress.getAllByName() doesn't really returns the IP addresses in the right order according to java.net.preferIPv6Addresses setting. We might need to implement it ourselves.

No, it seems that I was wrong. It was just some issue with IntelliJ and JVM didn't pick up the java.net.preferIPv6Addresses network setting. Running from the command like works as expected: return order is aligned with the property value.

[anmolnar]

I've another idea folks. Wdyt about the following?

    private InetSocketAddress resolve(InetSocketAddress address) {
        try {
            String curHostString = address.getHostString();
            List<InetAddress> ra = new ArrayList<>(Arrays.asList(this.resolver.getAllByName(curHostString)));
            if (ra.isEmpty()) {
                return address;
            }
            Map<Boolean, List<InetAddress>> grouped = ra.stream()
                .collect(Collectors.groupingBy(ia -> ia instanceof Inet6Address));
            List<InetAddress> resolvedAddresses;
            if (Boolean.parseBoolean(System.getProperty("java.net.preferIPv6Addresses")) && !grouped.get(Boolean.TRUE).isEmpty()) {
                resolvedAddresses = grouped.get(Boolean.TRUE);   // we prefer v6, and we have at least one v6 address available
            } else {
                resolvedAddresses = grouped.get(Boolean.FALSE);  // we prefer v4 or we only have v4 addresses available
            }
            Collections.shuffle(resolvedAddresses);
            return new InetSocketAddress(resolvedAddresses.get(0), address.getPort());
        } catch (UnknownHostException e) {
            LOG.error("Unable to resolve address: {}", address.toString(), e);
            return address;
        }
    }

This way we keep the random pick behavior, but first we group the IPs based on JVM setting. This might be the most direct solution of the problem and it doesn't rely on getAllByName() behavior, but at the same time doesn't follow it if influenced by some other setting.

I think we don't need feature flag with this approach.

[phunt]

+1 - lgtm and esp appreciate the time/effort to maintain the original behavior as possible. Docs are great to see. thx!

[anmolnar]

@kezhuw @phunt Did you check my alternative approach in the above comment?
I can go ahead and submit the PR as it is, but wanna make sure you didn't miss it.

[phunt]

Your most recent comment sounds reasonable - but it seems like having a big "on/off" for the new functionality would be the safest. (which is not inconsistent with this iiuc?)

[kezhuw]

The alternative approach violates "system" of "java.net.preferIPv6Addresses".

[anmolnar]

Ok. Thanks for the feedback. Let's merge it as it is.