[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r191252989
--- Diff: src/java/test/org/apache/zookeeper/test/ClientPortBindTest.java
---
@@ -102,9 +102,9 @@ public void testBindByAddress() throws Exception {
startSignal = new CountDownLatch(1);
ZooKeeper zk = new ZooKeeper(HOSTPORT, CONNECTION_TIMEOUT, this);
try {
-startSignal.await(CONNECTION_TIMEOUT,
--- End diff --
Sure, I agree. I think that was a suggestion from @afine and it's
completely reasonable.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar closed the pull request at: https://github.com/apache/zookeeper/pull/451 ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r191243666
--- Diff: src/java/test/org/apache/zookeeper/test/ClientPortBindTest.java
---
@@ -102,9 +102,9 @@ public void testBindByAddress() throws Exception {
startSignal = new CountDownLatch(1);
ZooKeeper zk = new ZooKeeper(HOSTPORT, CONNECTION_TIMEOUT, this);
try {
-startSignal.await(CONNECTION_TIMEOUT,
--- End diff --
The change in this file sounds not related to this patch. Usually we tend
to limit changes only relevant to a patch. Similar for the change in
ReadOnlyModeTest file. It's fine in this case as both changes are small and are
good improvements.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user hanm commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r191242849 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -21,6 +21,7 @@ import org.apache.yetus.audience.InterfaceAudience; import java.net.InetSocketAddress; +import java.net.UnknownHostException; --- End diff -- This import is not used, we can remove it. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r191087859 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -53,7 +54,7 @@ * @param spinDelay *Milliseconds to wait if all hosts have been tried once. */ -public InetSocketAddress next(long spinDelay); +public InetSocketAddress next(long spinDelay) throws UnknownHostException; --- End diff -- That's actually a great idea. I've tried that and `socket.connect()` will throw an IOException eventually of the address is not resolved. Which means basically that the same retry logic applies in the caller, it's just not next() that catches the error. I'll update the patch. Once this one is finally merged, I'll take a look at 3.5/master branches, because I think we should be consistent with this across the board. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user hanm commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r191034929 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -53,7 +54,7 @@ * @param spinDelay *Milliseconds to wait if all hosts have been tried once. */ -public InetSocketAddress next(long spinDelay); +public InetSocketAddress next(long spinDelay) throws UnknownHostException; --- End diff -- >> In a nutshell the problem of unresolvable DNS name must be handled somewhere and here're the considerations I agree with this, however this can be done without throwing the UnknownHostException here. We can just return the curAddr in case of resolving failure, and since curAddr is not resolved, trying to connect to it on caller side will not succeed and caller can continue retry etc with existing logic. The primary concern here is change of a public API which might require clients (e.g. HBase) to recompile with ZK, so I prefer the approach that can achieve same goal (let caller handle retry logic) w/o changing the signature of a public API (alternatively we can throw an unchecked exception but semantically that sounds no better than UnknownHostException). So I am thinking the semantic of next() is: * If we can resolve an address, resolve it and return. * If we can't resolve an address, return an unresolved address which contain enough information (host, port etc) for caller to decide what to do. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user hanm commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r191034452 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -30,76 +31,118 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; - /** * Most simple HostProvider, resolves only on instantiation. --- End diff -- Now we don't only resolve on instantiation, so this comment needs update. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r190899356 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -53,7 +54,7 @@ * @param spinDelay *Milliseconds to wait if all hosts have been tried once. */ -public InetSocketAddress next(long spinDelay); +public InetSocketAddress next(long spinDelay) throws UnknownHostException; --- End diff -- @hanm This is done. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r190846274 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -53,7 +54,7 @@ * @param spinDelay *Milliseconds to wait if all hosts have been tried once. */ -public InetSocketAddress next(long spinDelay); +public InetSocketAddress next(long spinDelay) throws UnknownHostException; --- End diff -- @hanm Answering @fpj comment below: > > We need a way to break the loop in the case the client closes, though. > That's actually a good reason for _not_ dealing with the error here. Because the caller - ClientCnxn - is be able to detect client closes, but StatisHostProvider is not. In a nutshell the problem of unresolvable DNS name must be handled somewhere and here're the considerations: 1. Client should endlessly try to resolve DNS names instead of giving up at some point. The re-try logic is already implemented in the caller methods properly. 2. In order to avoid API change we have to replicate the retry logic in the next() method which would add more complexity to the method and more extensive testing With these considerations I strongly believe that the API change is reasonable. That's basically what we discussed in the e-mail thread too. I'll update the comment as requested. Thanks. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user hanm commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r190789369 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -53,7 +54,7 @@ * @param spinDelay *Milliseconds to wait if all hosts have been tried once. */ -public InetSocketAddress next(long spinDelay); +public InetSocketAddress next(long spinDelay) throws UnknownHostException; --- End diff -- Some questions here @anmolnar: >> See my comment below. Now I think the API change is necessary, but I need to double check how it was detected previously. Which comment? I had a hard time find it, could you please explicitly paste it here? >> I started a thread on the mailing list a while ago for exactly the same reason. Let me update it. Similarly I lost that email thread. Could you please describe the gist of the email? I just wan to make sure we all agree that first an API change here is needed and second, if the API changes we need throw the right type of exception. (Also note the current comment of HostProvider interface should be updated if the API changes, because previous semantic of next always assume returns a "valid" address.). ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186759876
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
--- End diff --
Ok.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186741712
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -30,76 +31,118 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-
/**
* Most simple HostProvider, resolves only on instantiation.
- *
+ *
*/
@InterfaceAudience.Public
public final class StaticHostProvider implements HostProvider {
+public interface Resolver {
+InetAddress[] getAllByName(String name) throws
UnknownHostException;
+}
+
private static final Logger LOG = LoggerFactory
.getLogger(StaticHostProvider.class);
-private final List serverAddresses = new
ArrayList(
-5);
+private final List serverAddresses = new
ArrayList(5);
private int lastIndex = -1;
private int currentIndex = -1;
+private Resolver resolver;
+
/**
* Constructs a SimpleHostSet.
- *
+ *
* @param serverAddresses
*possibly unresolved ZooKeeper server addresses
* @throws IllegalArgumentException
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection
serverAddresses) {
-for (InetSocketAddress address : serverAddresses) {
-try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
-for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
-}
-} catch (UnknownHostException e) {
-LOG.error("Unable to connect to server: {}", address, e);
+this.resolver = new Resolver() {
+@Override
+public InetAddress[] getAllByName(String name) throws
UnknownHostException {
+return InetAddress.getAllByName(name);
}
-}
-
-if (this.serverAddresses.isEmpty()) {
+};
+init(serverAddresses);
+}
+
+/**
+ * Introduced for testing purposes. getAllByName() is a static method
of InetAddress, therefore cannot be easily mocked.
+ * By abstraction of Resolver interface we can easily inject a mocked
implementation in tests.
+ *
+ * @param serverAddresses
+ *possibly unresolved ZooKeeper server addresses
+ * @param resolver
+ *custom resolver implementation
+ * @throws IllegalArgumentException
+ * if serverAddresses is empty or resolves to an empty list
+ */
+public StaticHostProvider(Collection
serverAddresses, Resolver resolver) {
+this.resolver = resolver;
+init(serverAddresses);
+}
+
+/**
+ * Common init method for all constructors.
+ * Resolve all unresolved server addresses, put them in a list and
shuffle.
+ */
+private void init(Collection serverAddresses) {
+if (serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
+
+this.serverAddresses.addAll(serverAddresses);
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r186735649 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -53,7 +54,7 @@ * @param spinDelay *Milliseconds to wait if all hosts have been tried once. */ -public InetSocketAddress next(long spinDelay); +public InetSocketAddress next(long spinDelay) throws UnknownHostException; --- End diff -- 1- See my comment below. Now I think the API change is necessary, but I need to double check how it was detected previously. 2- I started a thread on the mailing list a while ago for exactly the same reason. Let me update it. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186734788
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
--- End diff --
> We need a way to break the loop in the case the client closes, though.
That's actually a good reason for _not_ dealing with the error here.
Because the caller - ClientCnxn - is be able to detect client closes, but
StatisHostProvider is not.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186694289
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -30,76 +31,118 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-
/**
* Most simple HostProvider, resolves only on instantiation.
- *
+ *
*/
@InterfaceAudience.Public
public final class StaticHostProvider implements HostProvider {
+public interface Resolver {
+InetAddress[] getAllByName(String name) throws
UnknownHostException;
+}
+
private static final Logger LOG = LoggerFactory
.getLogger(StaticHostProvider.class);
-private final List serverAddresses = new
ArrayList(
-5);
+private final List serverAddresses = new
ArrayList(5);
private int lastIndex = -1;
private int currentIndex = -1;
+private Resolver resolver;
+
/**
* Constructs a SimpleHostSet.
- *
+ *
* @param serverAddresses
*possibly unresolved ZooKeeper server addresses
* @throws IllegalArgumentException
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection
serverAddresses) {
-for (InetSocketAddress address : serverAddresses) {
-try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
-for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
-}
-} catch (UnknownHostException e) {
-LOG.error("Unable to connect to server: {}", address, e);
+this.resolver = new Resolver() {
+@Override
+public InetAddress[] getAllByName(String name) throws
UnknownHostException {
+return InetAddress.getAllByName(name);
}
-}
-
-if (this.serverAddresses.isEmpty()) {
+};
+init(serverAddresses);
+}
+
+/**
+ * Introduced for testing purposes. getAllByName() is a static method
of InetAddress, therefore cannot be easily mocked.
+ * By abstraction of Resolver interface we can easily inject a mocked
implementation in tests.
+ *
+ * @param serverAddresses
+ *possibly unresolved ZooKeeper server addresses
+ * @param resolver
+ *custom resolver implementation
+ * @throws IllegalArgumentException
+ * if serverAddresses is empty or resolves to an empty list
+ */
+public StaticHostProvider(Collection
serverAddresses, Resolver resolver) {
+this.resolver = resolver;
+init(serverAddresses);
+}
+
+/**
+ * Common init method for all constructors.
+ * Resolve all unresolved server addresses, put them in a list and
shuffle.
+ */
+private void init(Collection serverAddresses) {
+if (serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
+
+this.serverAddresses.addAll(serverAddresses);
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186690327
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
+}
+Collections.shuffle(resolvedAddresses);
--- End diff --
Ok.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186690242
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
--- End diff --
I think it is fine to loop indefinitely because the client needs it for
progress. We need a way to break the loop in the case the client closes, though.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r186689865 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -53,7 +54,7 @@ * @param spinDelay *Milliseconds to wait if all hosts have been tried once. */ -public InetSocketAddress next(long spinDelay); +public InetSocketAddress next(long spinDelay) throws UnknownHostException; --- End diff -- My preference is the following: 1- That we do not make this API change. It is not necessary to solve the problem of this issue. 2- That we discuss separately whether we want to change the behaviour of the `next()` in the `HostProvider` interface. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186591847
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
+}
+Collections.shuffle(resolvedAddresses);
--- End diff --
Correctly configured DNS servers return multiple IP addresses in round
robin fashion for a single hostname, so shuffle is not needed. However there
was a concern of badly configured DNS servers might not do this properly and we
want to workaround it here.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186591545
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
--- End diff --
I believe the expectation - and the original behaviour- is to loop forever
until the DNS problem is fixed. Even if we don't do it here, the caller will.
I'd loop here infinitely. Also should keep an eye on spinDelays to avoid
hammering the logic.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r186591140 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -53,7 +54,7 @@ * @param spinDelay *Milliseconds to wait if all hosts have been tried once. */ -public InetSocketAddress next(long spinDelay); +public InetSocketAddress next(long spinDelay) throws UnknownHostException; --- End diff -- That's a very good point actually, can't remember why implemented this way. I checked the callers' side and both fall into the same try-catch report loop which reports the problem and retry calling `next()` method. I think the reason was to keep the logic simple here and don't implement something which is already handled by someone else. But because it's a change on a public API, it would be probably better to do the retry logic here. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186553219
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
+}
+Collections.shuffle(resolvedAddresses);
--- End diff --
It doesn't't hurt, but do we really need to shuffle the resolved addresses?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r186546855 --- Diff: src/java/main/org/apache/zookeeper/client/HostProvider.java --- @@ -53,7 +54,7 @@ * @param spinDelay *Milliseconds to wait if all hosts have been tried once. */ -public InetSocketAddress next(long spinDelay); +public InetSocketAddress next(long spinDelay) throws UnknownHostException; --- End diff -- It feels odd to throw unknown host here because `next` is supposed to return a resolved host. The caller is asking for next resolved, not to resolve a given host name. Why do we need to throw it here and assuming we do need to throw something here, is there an exception that captures better the error you are trying to propagate? ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r186553004
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -111,9 +154,18 @@ public InetSocketAddress next(long spinDelay) {
lastIndex = 0;
}
-return serverAddresses.get(currentIndex);
+InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+
+String curHostString = getHostString(curAddr);
+List resolvedAddresses = new
ArrayList(Arrays.asList(this.resolver.getAllByName(curHostString)));
+if (resolvedAddresses.isEmpty()) {
+throw new UnknownHostException("No IP address returned for
address: " + curHostString);
--- End diff --
Should we try with the next on the list rather than throwing? After all,
the call is to get the "next" resolved address. Of course, it is possible that
we go through the whole list an find nothing. We might want to throw then so
that we don't loop forever.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168764368
--- Diff:
src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java ---
@@ -119,6 +120,68 @@ public void testTwoInvalidHostAddresses() {
new StaticHostProvider(list);
}
+@Test
+public void testReResolvingSingle() {
+byte size = 1;
+ArrayList list = new
ArrayList(size);
+
+// Test a hostname that resolves to a single address
+list.clear();
--- End diff --
Removed.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168763559
--- Diff: src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java ---
@@ -239,13 +243,13 @@ public void testSessionEstablishment() throws
Exception {
public void testSeekForRwServer() throws Exception {
// setup the logger to capture all logs
-Layout layout = Logger.getRootLogger().getAppender("CONSOLE")
+Layout layout =
org.apache.log4j.Logger.getRootLogger().getAppender("CONSOLE")
--- End diff --
Makes sense. I revert the changes.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168580127
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168575513
--- Diff: src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java ---
@@ -239,13 +243,13 @@ public void testSessionEstablishment() throws
Exception {
public void testSeekForRwServer() throws Exception {
// setup the logger to capture all logs
-Layout layout = Logger.getRootLogger().getAppender("CONSOLE")
+Layout layout =
org.apache.log4j.Logger.getRootLogger().getAppender("CONSOLE")
--- End diff --
I guess my real question is, do we need any of the changes in this file?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168578617
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168568596
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -47,59 +51,169 @@
private int currentIndex = -1;
+// Don't re-resolve on first next() call
+private boolean connectedSinceNext = true;
+
+private Resolver resolver;
+
/**
* Constructs a SimpleHostSet.
- *
+ *
* @param serverAddresses
*possibly unresolved ZooKeeper server addresses
* @throws IllegalArgumentException
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection
serverAddresses) {
+this.resolver = new Resolver() {
+@Override
+public InetAddress[] getAllByName(String name) throws
UnknownHostException {
+return InetAddress.getAllByName(name);
+}
+};
+init(serverAddresses);
+}
+
+/**
+ * Introduced for testing purposes. getAllByName() is a static method
of InetAddress, therefore cannot be easily mocked.
+ * By abstraction of Resolver interface we can easily inject a mocked
implementation in tests.
+ *
+ * @param serverAddresses
+ *possibly unresolved ZooKeeper server addresses
+ * @param resolver
+ *custom resolver implementation
+ * @throws IllegalArgumentException
+ * if serverAddresses is empty or resolves to an empty list
+ */
+public StaticHostProvider(Collection
serverAddresses, Resolver resolver) {
+this.resolver = resolver;
+init(serverAddresses);
+}
+
+/**
+ * Common init method for all constructors.
+ * Resolve all unresolved server addresses, put them in a list and
shuffle.
+ */
+private void init(Collection serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
this.resolver.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
+
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168572502
--- Diff: src/java/test/org/apache/zookeeper/test/ClientPortBindTest.java
---
@@ -104,7 +104,7 @@ public void testBindByAddress() throws Exception {
try {
startSignal.await(CONNECTION_TIMEOUT,
TimeUnit.MILLISECONDS);
-Assert.assertTrue("count == 0", startSignal.getCount() == 0);
+Assert.assertTrue("count == " + startSignal.getCount(),
startSignal.getCount() == 0);
--- End diff --
I'm not a huge fan of calling `getCount` more than once, since I think the
value could change between invocations. Why not just use the return value from
`await` in the line above?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168576090
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -47,59 +51,169 @@
private int currentIndex = -1;
+// Don't re-resolve on first next() call
+private boolean connectedSinceNext = true;
+
+private Resolver resolver;
+
/**
* Constructs a SimpleHostSet.
- *
+ *
* @param serverAddresses
*possibly unresolved ZooKeeper server addresses
* @throws IllegalArgumentException
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection
serverAddresses) {
+this.resolver = new Resolver() {
+@Override
+public InetAddress[] getAllByName(String name) throws
UnknownHostException {
+return InetAddress.getAllByName(name);
+}
+};
+init(serverAddresses);
+}
+
+/**
+ * Introduced for testing purposes. getAllByName() is a static method
of InetAddress, therefore cannot be easily mocked.
+ * By abstraction of Resolver interface we can easily inject a mocked
implementation in tests.
+ *
+ * @param serverAddresses
+ *possibly unresolved ZooKeeper server addresses
+ * @param resolver
+ *custom resolver implementation
+ * @throws IllegalArgumentException
+ * if serverAddresses is empty or resolves to an empty list
+ */
+public StaticHostProvider(Collection
serverAddresses, Resolver resolver) {
+this.resolver = resolver;
+init(serverAddresses);
+}
+
+/**
+ * Common init method for all constructors.
+ * Resolve all unresolved server addresses, put them in a list and
shuffle.
+ */
+private void init(Collection serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
this.resolver.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
+
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168576884
--- Diff:
src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java ---
@@ -119,6 +120,68 @@ public void testTwoInvalidHostAddresses() {
new StaticHostProvider(list);
}
+@Test
+public void testReResolvingSingle() {
+byte size = 1;
+ArrayList list = new
ArrayList(size);
+
+// Test a hostname that resolves to a single address
+list.clear();
--- End diff --
do we need this?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r168565039 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -18,6 +18,10 @@ package org.apache.zookeeper.client; +import org.apache.yetus.audience.InterfaceAudience; --- End diff -- nit: move this back ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r168564981 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -18,6 +18,10 @@ package org.apache.zookeeper.client; +import org.apache.yetus.audience.InterfaceAudience; --- End diff -- nit: move this back ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r168524336
--- Diff:
src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java ---
@@ -117,8 +116,32 @@ public void testTwoInvalidHostAddresses() {
list.add(new InetSocketAddress("a", 2181));
list.add(new InetSocketAddress("b", 2181));
new StaticHostProvider(list);
+ }
+
+@Test
+public void testReResolving() {
+byte size = 1;
+ArrayList list = new
ArrayList(size);
+
+// Test a hostname that resolves to multiple addresses
+list.add(InetSocketAddress.createUnresolved("www.apache.org",
1234));
--- End diff --
This is done.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r167027624
--- Diff:
src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java ---
@@ -117,8 +116,32 @@ public void testTwoInvalidHostAddresses() {
list.add(new InetSocketAddress("a", 2181));
list.add(new InetSocketAddress("b", 2181));
new StaticHostProvider(list);
+ }
+
+@Test
+public void testReResolving() {
+byte size = 1;
+ArrayList list = new
ArrayList(size);
+
+// Test a hostname that resolves to multiple addresses
+list.add(InetSocketAddress.createUnresolved("www.apache.org",
1234));
--- End diff --
Makes sense, I'm working on it.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r167024767
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r167013851
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166671671
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166670125
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r19488
--- Diff: src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java ---
@@ -239,13 +243,13 @@ public void testSessionEstablishment() throws
Exception {
public void testSeekForRwServer() throws Exception {
// setup the logger to capture all logs
-Layout layout = Logger.getRootLogger().getAppender("CONSOLE")
+Layout layout =
org.apache.log4j.Logger.getRootLogger().getAppender("CONSOLE")
--- End diff --
slf4j's string formatter feature is being used in these tests. I wouldn't
refactor them to be honest.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166479687
--- Diff:
src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java ---
@@ -117,8 +116,32 @@ public void testTwoInvalidHostAddresses() {
list.add(new InetSocketAddress("a", 2181));
list.add(new InetSocketAddress("b", 2181));
new StaticHostProvider(list);
+ }
+
+@Test
+public void testReResolving() {
+byte size = 1;
+ArrayList list = new
ArrayList(size);
+
+// Test a hostname that resolves to multiple addresses
+list.add(InetSocketAddress.createUnresolved("www.apache.org",
1234));
--- End diff --
Yeah, this is annoying. Although, another possibility would be to put the
dns calls in a method and then subclass `StaticHostProvider` for the tests and
overriding this method to return what you want.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166476703
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166476047
--- Diff: src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java ---
@@ -239,13 +243,13 @@ public void testSessionEstablishment() throws
Exception {
public void testSeekForRwServer() throws Exception {
// setup the logger to capture all logs
-Layout layout = Logger.getRootLogger().getAppender("CONSOLE")
+Layout layout =
org.apache.log4j.Logger.getRootLogger().getAppender("CONSOLE")
--- End diff --
not going to push too hard on this, but I think you can just use log4j
everywhere like in `QuorumPeerMainTest`
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166341540
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166329760
--- Diff:
src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java ---
@@ -117,8 +116,32 @@ public void testTwoInvalidHostAddresses() {
list.add(new InetSocketAddress("a", 2181));
list.add(new InetSocketAddress("b", 2181));
new StaticHostProvider(list);
+ }
+
+@Test
+public void testReResolving() {
+byte size = 1;
+ArrayList list = new
ArrayList(size);
+
+// Test a hostname that resolves to multiple addresses
+list.add(InetSocketAddress.createUnresolved("www.apache.org",
1234));
--- End diff --
Good point I spent hours finding a way for it. Unfortunately static members
can't be mocked with Mockito. We should use PowerMock for it (as described here
https://blog.codecentric.de/en/2016/03/junit-testing-using-mockito-powermock/),
but that'd be a new dependency not sure if it's acceptable here.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166328583
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r166286605 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -6,9 +6,9 @@ * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * + * --- End diff -- Yes it was. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166286530
--- Diff: src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java ---
@@ -239,13 +243,13 @@ public void testSessionEstablishment() throws
Exception {
public void testSeekForRwServer() throws Exception {
// setup the logger to capture all logs
-Layout layout = Logger.getRootLogger().getAppender("CONSOLE")
+Layout layout =
org.apache.log4j.Logger.getRootLogger().getAppender("CONSOLE")
--- End diff --
I few lines above there're org.apache.log4j references which conflicts with
the 'Logger' class, hence it needs to be explicitly referenced.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r166097161 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -6,9 +6,9 @@ * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * + * --- End diff -- was this accidental? ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166103404
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166105622
--- Diff:
src/java/test/org/apache/zookeeper/test/StaticHostProviderTest.java ---
@@ -117,8 +116,32 @@ public void testTwoInvalidHostAddresses() {
list.add(new InetSocketAddress("a", 2181));
list.add(new InetSocketAddress("b", 2181));
new StaticHostProvider(list);
+ }
+
+@Test
+public void testReResolving() {
+byte size = 1;
+ArrayList list = new
ArrayList(size);
+
+// Test a hostname that resolves to multiple addresses
+list.add(InetSocketAddress.createUnresolved("www.apache.org",
1234));
--- End diff --
I'm wondering if it's possible to mock this out? It would be great if our
unit tests were not dependent on some other infrastructure.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r166102194
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -58,48 +61,122 @@
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
+InetAddress resolvedAddresses[] =
InetAddress.getAllByName(getHostString(address));
for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
+this.serverAddresses.add(new
InetSocketAddress(resolvedAddress, address.getPort()));
}
} catch (UnknownHostException e) {
LOG.error("Unable to connect to server: {}", address, e);
}
}
-
+
if (this.serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
}
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+public int getNextAdded() {
+return nextAdded;
+}
+
+public int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r165666521 --- Diff: src/java/test/org/apache/zookeeper/client/StaticHostProviderTest.java --- @@ -16,7 +16,7 @@ * limitations under the License. */ -package org.apache.zookeeper.test; +package org.apache.zookeeper.client; --- End diff -- It was move to client package, because it uses package-private methods of StaticHostProvider. I moved back to 'test' package and change the affected methods to public. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r165665505
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -91,15 +79,106 @@ public
StaticHostProvider(Collection serverAddresses) {
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+int getNextAdded() {
+return nextAdded;
+}
+
+int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!connectedSinceNext) {
--- End diff --
It should try to re-resolve whenever the client is unable to connect to a
server (connectedSinceNext == false).
@fpj gives a good explanation in the original Jira:
https://issues.apache.org/jira/browse/ZOOKEEPER-2184?focusedCommentId=15873730&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15873730
> I haven't had much time to work on this issue, but here is my current
assessment.
> This issue seemed easy to fix at first, but it is fairly fundamental with
respect to how we resolve host names. Currently, we resolve host names when we
start a client and never resolve it again. This is the cause of the problem
reported in the issue because in the scenario described, the zookeeper
container is re-started and changes addresses, which prevents the client from
connecting to the zookeeper server.
> The proposed patch here tries to re-resolve the hostname every time the
client fails to connect to the resolved address. It kind of works, but it makes
StaticHostProvider a bit messy because the expectation with the current wiring
is that we won't have to resolve again.
> The ideal situation for the problematic scenario is that we resolve the
host name every time we try to connect to a server, but that would be a fairly
fundamental change to how we resolve addresses in ZooKeeper.
> I was also looking at the C client and it might get a bit messy too there
because I don't think we currently keep the association between the host name
and the resolved address, so we don't really know what to resolve again. It
might be possible to do it via the canonical name in getaddrinfo, but I'm not
sure how that works with windows.
> One specific proposal to avoid having clients never finding a server ever
again without deep changes to the current wiring is to resolve again everything
in the case the client tries all and none succeeds. That would be a fairly
straightforward change to both Java and C client, but it would not resolve
addresses again in the case the a strict subset has changed addresses and at
least one server is reachable.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r165658965
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -57,29 +62,12 @@
*/
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
-try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
-for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
-}
-} catch (UnknownHostException e) {
+ try {
--- End diff --
Fixing.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r165658743 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -25,6 +25,8 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.lang.reflect.InvocationTargetException; --- End diff -- Good catch, thanks. ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r165527366
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -57,29 +62,12 @@
*/
public StaticHostProvider(Collection
serverAddresses) {
for (InetSocketAddress address : serverAddresses) {
-try {
-InetAddress ia = address.getAddress();
-InetAddress resolvedAddresses[] =
InetAddress.getAllByName((ia != null) ? ia.getHostAddress() :
-address.getHostName());
-for (InetAddress resolvedAddress : resolvedAddresses) {
-// If hostName is null but the address is not, we can
tell that
-// the hostName is an literal IP address. Then we can
set the host string as the hostname
-// safely to avoid reverse DNS lookup.
-// As far as i know, the only way to check if the
hostName is null is use toString().
-// Both the two implementations of InetAddress are
final class, so we can trust the return value of
-// the toString() method.
-if (resolvedAddress.toString().startsWith("/")
-&& resolvedAddress.getAddress() != null) {
-this.serverAddresses.add(
-new
InetSocketAddress(InetAddress.getByAddress(
-address.getHostName(),
-resolvedAddress.getAddress()),
-address.getPort()));
-} else {
-this.serverAddresses.add(new
InetSocketAddress(resolvedAddress.getHostAddress(), address.getPort()));
-}
-}
-} catch (UnknownHostException e) {
+ try {
--- End diff --
something is wrong with the indentation here
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r165525652
--- Diff: src/java/test/org/apache/zookeeper/test/ReadOnlyModeTest.java ---
@@ -239,13 +243,13 @@ public void testSessionEstablishment() throws
Exception {
public void testSeekForRwServer() throws Exception {
// setup the logger to capture all logs
-Layout layout = Logger.getRootLogger().getAppender("CONSOLE")
+Layout layout =
org.apache.log4j.Logger.getRootLogger().getAppender("CONSOLE")
--- End diff --
why is this necessary?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r165524377 --- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java --- @@ -25,6 +25,8 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.lang.reflect.InvocationTargetException; --- End diff -- i think these imports are unused, and there are some others elsewhere in the code ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/451#discussion_r165525113 --- Diff: src/java/test/org/apache/zookeeper/client/StaticHostProviderTest.java --- @@ -16,7 +16,7 @@ * limitations under the License. */ -package org.apache.zookeeper.test; +package org.apache.zookeeper.client; --- End diff -- this doesn't look right ---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r165529085
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java
---
@@ -91,15 +79,106 @@ public
StaticHostProvider(Collection serverAddresses) {
Collections.shuffle(this.serverAddresses);
}
+/**
+ * Evaluate to a hostname if one is available and otherwise it returns
the
+ * string representation of the IP address.
+ *
+ * In Java 7, we have a method getHostString, but earlier versions do
not support it.
+ * This method is to provide a replacement for
InetSocketAddress.getHostString().
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+private String getHostString(InetSocketAddress addr) {
+String hostString = "";
+
+if (addr == null) {
+return hostString;
+}
+if (!addr.isUnresolved()) {
+InetAddress ia = addr.getAddress();
+
+// If the string starts with '/', then it has no hostname
+// and we want to avoid the reverse lookup, so we return
+// the string representation of the address.
+if (ia.toString().startsWith("/")) {
+hostString = ia.getHostAddress();
+} else {
+hostString = addr.getHostName();
+}
+} else {
+// According to the Java 6 documentation, if the hostname is
+// unresolved, then the string before the colon is the
hostname.
+String addrString = addr.toString();
+hostString = addrString.substring(0,
addrString.lastIndexOf(':'));
+}
+
+return hostString;
+}
+
public int size() {
return serverAddresses.size();
}
+// Counts the number of addresses added and removed during
+// the last call to next. Used mainly for test purposes.
+// See StasticHostProviderTest.
+private int nextAdded = 0;
+private int nextRemoved = 0;
+
+int getNextAdded() {
+return nextAdded;
+}
+
+int getNextRemoved() {
+return nextRemoved;
+}
+
public InetSocketAddress next(long spinDelay) {
-++currentIndex;
-if (currentIndex == serverAddresses.size()) {
-currentIndex = 0;
+// Handle possible connection error by re-resolving hostname if
possible
+if (!connectedSinceNext) {
--- End diff --
would you mind explaining exactly under which conditions we reresolve the
hostname and under which conditions we try the next one in the host list? My
reading is that this reresolves everything if the client fails to connect to
two hosts in a row. Is this the desired behavior?
And do we always reresolve all serverAddresses?
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/451#discussion_r163570637
--- Diff:
src/java/test/org/apache/zookeeper/client/StaticHostProviderTest.java ---
@@ -117,8 +117,32 @@ public void testTwoInvalidHostAddresses() {
list.add(new InetSocketAddress("a", 2181));
list.add(new InetSocketAddress("b", 2181));
new StaticHostProvider(list);
+ }
+
+@Test
+public void testReResolving() {
+byte size = 1;
+ArrayList list = new
ArrayList(size);
+
+// Test a hostname that resolves to multiple addresses
--- End diff --
Common domain names like facebook.com, google.com or apache.org don't use
round-robin DNS anymore for some reason, so they don't resolve to multiple
addresses. As a consequence this test doesn't validate the branch related to
multiple addresses anymore unfortunately.
Not sure how to address that, PowerMock would be the best to mock static
`getAllByName()` method, but that would involve introducing a new test
dependency.
---
[GitHub] zookeeper pull request #451: ZOOKEEPER-2184: Zookeeper Client should re-reso...
GitHub user anmolnar opened a pull request: https://github.com/apache/zookeeper/pull/451 ZOOKEEPER-2184: Zookeeper Client should re-resolve hosts when connection attempts fail This one is the pick-up of @fpj 's original PR: #150 Targeting and rebased on the 3.4 branch. You can merge this pull request into a Git repository by running: $ git pull https://github.com/anmolnar/zookeeper ZOOKEEPER-2184 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/451.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #451 commit 229760d7757f47e271a8e059c1aeac10f0847a2a Author: fpj Date: 2017-01-14T16:58:15Z ZOOKEEPER-2184: Zookeeper Client should re-resolve hosts when connection attempts fail ---
