You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by anmolnar <gi...@git.apache.org> on 2018/05/29 15:21:40 UTC
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
GitHub user anmolnar opened a pull request:
https://github.com/apache/zookeeper/pull/534
ZOOKEEPER-2184 Zookeeper Client should re-resolve hosts when connection attempts fail
This is the master/3.5 port of #451
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anmolnar/zookeeper ZOOKEEPER-2184_master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/534.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 #534
----
commit 98ea0044ebf7c3773d10001b7ace8e8e74ec94ce
Author: Andor Molnar <an...@...>
Date: 2018-05-29T15:19:15Z
ZOOKEEPER-2184. Ported the patch from branch-3.4
----
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/534
---
[GitHub] zookeeper issue #534: ZOOKEEPER-2184 Zookeeper Client should re-resolve host...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/534
Unit test refactoring: https://issues.apache.org/jira/browse/ZOOKEEPER-3065
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r192630799
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -96,36 +115,46 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses) {
*/
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses,
long randomnessSeed) {
- sourceOfRandomness = new Random(randomnessSeed);
+ init(serverAddresses, randomnessSeed, new Resolver() {
+ @Override
+ public InetAddress[] getAllByName(String name) throws UnknownHostException {
+ return InetAddress.getAllByName(name);
+ }
+ });
+ }
- this.serverAddresses = resolveAndShuffle(serverAddresses);
- if (this.serverAddresses.isEmpty()) {
+ private void init(Collection<InetSocketAddress> serverAddresses, long randomnessSeed, Resolver resolver) {
+ this.sourceOfRandomness = new Random(randomnessSeed);
+ this.resolver = resolver;
+ if (serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
- }
+ }
+ this.serverAddresses = shuffle(serverAddresses);
currentIndex = -1;
- lastIndex = -1;
+ lastIndex = -1;
}
- private List<InetSocketAddress> resolveAndShuffle(Collection<InetSocketAddress> serverAddresses) {
- List<InetSocketAddress> tmpList = new ArrayList<InetSocketAddress>(serverAddresses.size());
- for (InetSocketAddress address : serverAddresses) {
- try {
- InetAddress ia = address.getAddress();
- String addr = (ia != null) ? ia.getHostAddress() : address.getHostString();
- InetAddress resolvedAddresses[] = InetAddress.getAllByName(addr);
- for (InetAddress resolvedAddress : resolvedAddresses) {
- InetAddress taddr = InetAddress.getByAddress(address.getHostString(), resolvedAddress.getAddress());
- tmpList.add(new InetSocketAddress(taddr, address.getPort()));
- }
- } catch (UnknownHostException ex) {
- LOG.warn("No IP address found for server: {}", address, ex);
+ private InetSocketAddress resolve(InetSocketAddress address) {
+ try {
+ String curHostString = address.getHostString();
+ List<InetAddress> resolvedAddresses = new ArrayList<>(Arrays.asList(this.resolver.getAllByName(curHostString)));
+ if (resolvedAddresses.isEmpty()) {
+ return address;
}
+ Collections.shuffle(resolvedAddresses);
+ return new InetSocketAddress(resolvedAddresses.get(0), address.getPort());
+ } catch (UnknownHostException e) {
--- End diff --
Should log the exception here. Does the caller handle the unknown address correctly?
---
[GitHub] zookeeper issue #534: ZOOKEEPER-2184 Zookeeper Client should re-resolve host...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/534
>> it seems like we also introduced a findbug issue with #562
Let's do this as a separate issue. I committed the patch, so I feel obligated to fix it:
https://github.com/apache/zookeeper/pull/568
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r201456281
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -149,15 +185,12 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses,
* @param currentHost the host to which this client is currently connected
* @return true if changing connections is necessary for load-balancing, false otherwise
*/
-
-
@Override
public synchronized boolean updateServerList(
Collection<InetSocketAddress> serverAddresses,
InetSocketAddress currentHost) {
- // Resolve server addresses and shuffle them
- List<InetSocketAddress> resolvedList = resolveAndShuffle(serverAddresses);
- if (resolvedList.isEmpty()) {
+ List<InetSocketAddress> shuffledList = shuffle(serverAddresses);
--- End diff --
>> However, exactly the same functionality has already been implemented in the InetSocketAddress.equals() method
I am wondering if this is the case or not. I just did a random peek at https://github.com/openjdk-mirror/jdk7u-jdk/blob/master/src/share/classes/java/net/InetSocketAddress.java#L300, looks like if we compare an unresolved address with a resolved address, the equals will return false - but the code you pasted will return true if getHostString works for both resolved and unresolved address... could you double check this behavior?
Also, is it possible to add a test case to cover the case where the second parameter of updateServerList is a resolved address? The existing test cases only cover the case where the second parameter (myServer) is unresolved. In practice I think the method updateServerList is called by ZooKeeper's updateServerList method with second parameter as a resolved address (the remote server where current client connected to.).
---
[GitHub] zookeeper issue #534: ZOOKEEPER-2184 Zookeeper Client should re-resolve host...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/534
@anmolnar do you have a chance to address the issues from code review? Please let me know if you plan to work on this in next few days - otherwise I'll just go ahead and do the 3.4 release without waiting on this merged to 3.5 and master.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r192627337
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -73,15 +80,27 @@
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses) {
- sourceOfRandomness = new Random(System.currentTimeMillis() ^ this.hashCode());
+ init(serverAddresses,
--- End diff --
Nit: Call StaticHostProvider(Collection<InetSocketAddress> serverAddresses, Resolver resolver).
Please change the other two constructor as well.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r194185189
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -73,15 +80,27 @@
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses) {
- sourceOfRandomness = new Random(System.currentTimeMillis() ^ this.hashCode());
+ init(serverAddresses,
--- End diff --
I think what @lvfangmin meant is instead of duplicating the code inside three constructors, only keep one concrete parameterized constructor implementation and let the other two invoke that one (with different parameters.).
---
[GitHub] zookeeper issue #534: ZOOKEEPER-2184 Zookeeper Client should re-resolve host...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/534
@hanm Did you have a chance to take a look?
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r201723327
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -149,15 +185,12 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses,
* @param currentHost the host to which this client is currently connected
* @return true if changing connections is necessary for load-balancing, false otherwise
*/
-
-
@Override
public synchronized boolean updateServerList(
Collection<InetSocketAddress> serverAddresses,
InetSocketAddress currentHost) {
- // Resolve server addresses and shuffle them
- List<InetSocketAddress> resolvedList = resolveAndShuffle(serverAddresses);
- if (resolvedList.isEmpty()) {
+ List<InetSocketAddress> shuffledList = shuffle(serverAddresses);
--- End diff --
@hanm Good catch, that makes perfect sense. The implementation is slightly different.
I added 4 new test cases to cover the different combinations of the 2nd and 3rd parameter. Some of them could be redundant (as you said tests present already), but I've already opened a Jira to clean-up this test file, so it should be okay. Test scenarios:
*Given:* list of servers contains 1 element, client is connected (currentHost (myServer) != null), trying to replace server list with the same address (replaceHost).
1. `currentHost` resolved, `replaceHost` unresolved => client should disconnect,
2. `currentHost` resolved, `replaceHost` resolved => client should *not* disconnect,
3. `currentHost` unresolved, `replaceHost` unresolved => client should disconnect,
4. `currentHost` unresolved, `replaceHost` resolved => client should disconnect
My implementation covers all of these scenarios. The only difference is in *case no.4* where the original impl returns false (client should not disconnect). Question is what we would like to do in the 4th case? Does it make sense at all, as you mentioned client should not be connected to an unresolved address?
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r192694235
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -73,15 +80,27 @@
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses) {
- sourceOfRandomness = new Random(System.currentTimeMillis() ^ this.hashCode());
+ init(serverAddresses,
--- End diff --
Sorry @lvfangmin , I might be missing the point here. Shall I change the signature to use non-generic Collection?
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r194215559
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -149,15 +185,12 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses,
* @param currentHost the host to which this client is currently connected
* @return true if changing connections is necessary for load-balancing, false otherwise
*/
-
-
@Override
public synchronized boolean updateServerList(
Collection<InetSocketAddress> serverAddresses,
InetSocketAddress currentHost) {
- // Resolve server addresses and shuffle them
- List<InetSocketAddress> resolvedList = resolveAndShuffle(serverAddresses);
- if (resolvedList.isEmpty()) {
+ List<InetSocketAddress> shuffledList = shuffle(serverAddresses);
--- End diff --
After this change, the old / new server list both contains unresolved address, so it's comparable, but it's meaningless to compare addr.getAddress because it will return null when it's unresolved.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r192630642
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -314,7 +340,7 @@ public InetSocketAddress next(long spinDelay) {
addr = nextHostInReconfigMode();
if (addr != null) {
currentIndex = serverAddresses.indexOf(addr);
- return addr;
+ return resolve(addr);
--- End diff --
Can you do some test to find out performance/latency of resolving an addr? If it's costly, maybe we should cache the resolved one instead of resolve it every time.
---
[GitHub] zookeeper issue #534: ZOOKEEPER-2184 Zookeeper Client should re-resolve host...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/534
I think this one is good to commit - I'll commit in next few days if no one raise other concerns.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r196049820
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -73,15 +80,27 @@
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses) {
- sourceOfRandomness = new Random(System.currentTimeMillis() ^ this.hashCode());
+ init(serverAddresses,
--- End diff --
Tried that. Cannot call the other constructor, because `this` is referenced.
---
[GitHub] zookeeper issue #534: ZOOKEEPER-2184 Zookeeper Client should re-resolve host...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/534
@hanm @lvfangmin Reconfig tests have previously been implemented only for IP addresses and hasn't covered hostnames which need to be resolved. I added a few unit tests for coverage, but found that existing tests should also be refactored.
I don't want to overload this patch, so will create a separate Jira for the refactoring instead.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r196052286
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -314,7 +340,7 @@ public InetSocketAddress next(long spinDelay) {
addr = nextHostInReconfigMode();
if (addr != null) {
currentIndex = serverAddresses.indexOf(addr);
- return addr;
+ return resolve(addr);
--- End diff --
IMHO JVM caching is overkill too. It's not application's responsibility to deal with DNS re-resolution, because it shouldn't happen in failure scenarios. On the flipside to avoid DNS server flooding some caching in the app would be reasonable and JVM is a standard way of doing that.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r192694788
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -314,7 +340,7 @@ public InetSocketAddress next(long spinDelay) {
addr = nextHostInReconfigMode();
if (addr != null) {
currentIndex = serverAddresses.indexOf(addr);
- return addr;
+ return resolve(addr);
--- End diff --
I disagree. We should not do any caching in our codebase, because there're multiple levels of caching already present in DNS infrastructure, like JVM caching, os-level caching, DNS servers caching, etc. `resolve()` will eventually become a no-op if any of these caches find a hit.
---
[GitHub] zookeeper issue #534: ZOOKEEPER-2184 Zookeeper Client should re-resolve host...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/534
@hanm Thanks, I updated the comments to be consistent with the original PR.
Unfortunately the comment of `HostProvider` wasn't updated, so I also made changes in there.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r194186665
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -149,15 +185,12 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses,
* @param currentHost the host to which this client is currently connected
* @return true if changing connections is necessary for load-balancing, false otherwise
*/
-
-
@Override
public synchronized boolean updateServerList(
Collection<InetSocketAddress> serverAddresses,
InetSocketAddress currentHost) {
- // Resolve server addresses and shuffle them
- List<InetSocketAddress> resolvedList = resolveAndShuffle(serverAddresses);
- if (resolvedList.isEmpty()) {
+ List<InetSocketAddress> shuffledList = shuffle(serverAddresses);
--- End diff --
I am not sure why we change the `resolveAndShuffle` to `shuffle` here, they are semantically different (one tries to resolve address, the other does not and only does shuffle.). The `serverAddresses` passed in this method is unresolved address, and we need it resolved because we rely on the resolved addresses to compare the old / new server list (in the context of probability rebalancing clients for dynamic reconfiguration).
Without resolving I think the client rebalance logic will be broken. A side note that all tests still passed probably indicate we don't have a 100% coverage for the logic in our tests.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r195964179
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -149,15 +185,12 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses,
* @param currentHost the host to which this client is currently connected
* @return true if changing connections is necessary for load-balancing, false otherwise
*/
-
-
@Override
public synchronized boolean updateServerList(
Collection<InetSocketAddress> serverAddresses,
InetSocketAddress currentHost) {
- // Resolve server addresses and shuffle them
- List<InetSocketAddress> resolvedList = resolveAndShuffle(serverAddresses);
- if (resolvedList.isEmpty()) {
+ List<InetSocketAddress> shuffledList = shuffle(serverAddresses);
--- End diff --
`resolveAndShuffle` has been split into two methods: `resolve` and `shuffle`, because we have to do it separately. Github shows the diff as original has been renamed and new one has been created.
I'll double check the rebalance functionality to make sure it's working properly and will also check related unit tests. I think what @lvfangmin is saying makes sense.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r201456782
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -73,15 +80,27 @@
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses) {
- sourceOfRandomness = new Random(System.currentTimeMillis() ^ this.hashCode());
+ init(serverAddresses,
--- End diff --
Yeah, I think referencing this before the object is constructed is prohibited. I think we can probably get around it but it looks like not worth the extra effort (I don't know a simple of way of doing it without dropping using the this.hashCode).
So I am ok with the current form of having all ctors invoking init method.
---
[GitHub] zookeeper issue #534: ZOOKEEPER-2184 Zookeeper Client should re-resolve host...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/534
@hanm @lvfangmin Are you happy with commiting the patch?
---
[GitHub] zookeeper issue #534: ZOOKEEPER-2184 Zookeeper Client should re-resolve host...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/534
@lvfangmin Unit tests failed, but it seems like we also introduced a findbug issue with #562
Shall I fix it in this patch or would you like to do it separately?
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r201894799
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -149,15 +185,12 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses,
* @param currentHost the host to which this client is currently connected
* @return true if changing connections is necessary for load-balancing, false otherwise
*/
-
-
@Override
public synchronized boolean updateServerList(
Collection<InetSocketAddress> serverAddresses,
InetSocketAddress currentHost) {
- // Resolve server addresses and shuffle them
- List<InetSocketAddress> resolvedList = resolveAndShuffle(serverAddresses);
- if (resolvedList.isEmpty()) {
+ List<InetSocketAddress> shuffledList = shuffle(serverAddresses);
--- End diff --
Thanks for quick update, @anmolnar !
>> The built-in implementation treats them as different address
That's what I am not sure about. If an unresolved address and a resolved address actually maps same address (after unresolved address gets resolved), should both be treated as same address (old behavior, via getHostString), instead of different address?
What I am thinking is to instead of using equal - use the old verbose code which will work (so far) for all combinations of comparing resolved and unresolved address. What do you think about this?
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r194215224
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -73,15 +80,27 @@
* if serverAddresses is empty or resolves to an empty list
*/
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses) {
- sourceOfRandomness = new Random(System.currentTimeMillis() ^ this.hashCode());
+ init(serverAddresses,
--- End diff --
What hanm said, instead of call init, call this.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r192696652
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -96,36 +115,46 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses) {
*/
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses,
long randomnessSeed) {
- sourceOfRandomness = new Random(randomnessSeed);
+ init(serverAddresses, randomnessSeed, new Resolver() {
+ @Override
+ public InetAddress[] getAllByName(String name) throws UnknownHostException {
+ return InetAddress.getAllByName(name);
+ }
+ });
+ }
- this.serverAddresses = resolveAndShuffle(serverAddresses);
- if (this.serverAddresses.isEmpty()) {
+ private void init(Collection<InetSocketAddress> serverAddresses, long randomnessSeed, Resolver resolver) {
+ this.sourceOfRandomness = new Random(randomnessSeed);
+ this.resolver = resolver;
+ if (serverAddresses.isEmpty()) {
throw new IllegalArgumentException(
"A HostProvider may not be empty!");
- }
+ }
+ this.serverAddresses = shuffle(serverAddresses);
currentIndex = -1;
- lastIndex = -1;
+ lastIndex = -1;
}
- private List<InetSocketAddress> resolveAndShuffle(Collection<InetSocketAddress> serverAddresses) {
- List<InetSocketAddress> tmpList = new ArrayList<InetSocketAddress>(serverAddresses.size());
- for (InetSocketAddress address : serverAddresses) {
- try {
- InetAddress ia = address.getAddress();
- String addr = (ia != null) ? ia.getHostAddress() : address.getHostString();
- InetAddress resolvedAddresses[] = InetAddress.getAllByName(addr);
- for (InetAddress resolvedAddress : resolvedAddresses) {
- InetAddress taddr = InetAddress.getByAddress(address.getHostString(), resolvedAddress.getAddress());
- tmpList.add(new InetSocketAddress(taddr, address.getPort()));
- }
- } catch (UnknownHostException ex) {
- LOG.warn("No IP address found for server: {}", address, ex);
+ private InetSocketAddress resolve(InetSocketAddress address) {
+ try {
+ String curHostString = address.getHostString();
+ List<InetAddress> resolvedAddresses = new ArrayList<>(Arrays.asList(this.resolver.getAllByName(curHostString)));
+ if (resolvedAddresses.isEmpty()) {
+ return address;
}
+ Collections.shuffle(resolvedAddresses);
+ return new InetSocketAddress(resolvedAddresses.get(0), address.getPort());
+ } catch (UnknownHostException e) {
--- End diff --
That's correct. The caller will end up getting UnknownHostException when trying to open the socket to the unresolvable address:
```
2018-06-04 12:31:26,022 [myid:huhuuhujkdshgfjksgd.com:2181] - WARN [main-SendThread(huhuuhujkdshgfjksgd.com:2181):ClientCnxn$SendThread@1237] - Session 0x0 for server huhuuhujkdshgfjksgd.com:2181, unexpected error, closing socket connection and attempting reconnect
java.nio.channels.UnresolvedAddressException
at sun.nio.ch.Net.checkAddress(Net.java:101)
at sun.nio.ch.SocketChannelImpl.connect(SocketChannelImpl.java:622)
at org.apache.zookeeper.ClientCnxnSocketNIO.registerAndConnect(ClientCnxnSocketNIO.java:275)
at org.apache.zookeeper.ClientCnxnSocketNIO.connect(ClientCnxnSocketNIO.java:285)
at org.apache.zookeeper.ClientCnxn$SendThread.startConnect(ClientCnxn.java:1091)
at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1133)
```
Logging makes sense, I added an error log entry to make it clear.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r194215321
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -314,7 +340,7 @@ public InetSocketAddress next(long spinDelay) {
addr = nextHostInReconfigMode();
if (addr != null) {
currentIndex = serverAddresses.indexOf(addr);
- return addr;
+ return resolve(addr);
--- End diff --
I know there is DNS caching, not aware that we have JVM or os caching for this, if we tested this is trivial, I totally agree we should keep this simple.
---
[GitHub] zookeeper issue #534: ZOOKEEPER-2184 Zookeeper Client should re-resolve host...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/534
@hanm Sorry for the delay. I'll work on it today.
---
[GitHub] zookeeper pull request #534: ZOOKEEPER-2184 Zookeeper Client should re-resol...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/534#discussion_r196050719
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -149,15 +185,12 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses,
* @param currentHost the host to which this client is currently connected
* @return true if changing connections is necessary for load-balancing, false otherwise
*/
-
-
@Override
public synchronized boolean updateServerList(
Collection<InetSocketAddress> serverAddresses,
InetSocketAddress currentHost) {
- // Resolve server addresses and shuffle them
- List<InetSocketAddress> resolvedList = resolveAndShuffle(serverAddresses);
- if (resolvedList.isEmpty()) {
+ List<InetSocketAddress> shuffledList = shuffle(serverAddresses);
--- End diff --
Comparison works for unresolved addresses too, because of the last if condition here:
```java
if (addr.getPort() == myServer.getPort()
&& ((addr.getAddress() != null
&& myServer.getAddress() != null && addr
.getAddress().equals(myServer.getAddress())) || addr
.getHostString().equals(myServer.getHostString()))) {
```
As long as `getHostString()` works for unresolved addresses, we're fine. However, exactly the same functionality has already been implemented in the `InetSocketAddress.equals()` method, so I refactored this part to use it.
---