You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by fpj <gi...@git.apache.org> on 2017/01/14 17:00:37 UTC
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
GitHub user fpj opened a pull request:
https://github.com/apache/zookeeper/pull/150
ZOOKEEPER-2184: Zookeeper Client should re-resolve hosts when connection attempts fail
This is a version of the patch for ZK-2184 for the 3.4 branch, compatible with Java 6.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/fpj/zookeeper ZK-2184
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/150.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 #150
----
commit fbaa47e3a96f166cfae45070b0724780f13714e9
Author: fpj <fp...@apache.org>
Date: 2017-01-14T16:58:15Z
ZOOKEEPER-2184: Zookeeper Client should re-resolve hosts when connection attempts fail
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96120265
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -45,6 +45,9 @@
private int currentIndex = -1;
+ // Don't re-resolve on first next() call
+ private boolean connectedSinceNext = true;
--- End diff --
Sincere question: it is worth making this field *volatile*?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by djenriquez <gi...@git.apache.org>.
Github user djenriquez commented on the issue:
https://github.com/apache/zookeeper/pull/150
Any reasons why this hasn't been merged yet or the attention given? I agree whole-heartedly with @rcillo, this is a gigantic feature for anyone depending on Zookeeper in the cloud running on immutable/disposal infrastructure.
Would love to get this merged, but it being 6 months old makes me wonder if it has been de-prioritized by project owners/collaborators? If so, can someone explain why has this been de-prioritized?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/150
@tophei Thanks for reporting this.
There's a patch actively being review to address the problem here: https://github.com/apache/zookeeper/pull/451
It would be great if you could try that one out and make sure it's actually a working fix.
Could you please do it by any chance?
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r97436195
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
- InetAddress ia = address.getAddress();
- InetAddress resolvedAddresses[] = InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
- address.getHostName());
+ InetAddress resolvedAddresses[];
+ try {
+ Method m = InetSocketAddress.class.getDeclaredMethod("getHostString");
+ m.setAccessible(true);
+ resolvedAddresses = InetAddress.getAllByName((String) m.invoke(address));
+ } catch (IllegalAccessException e) {
+ resolvedAddresses = InetAddress.getAllByName(getHostString(address));
+ } catch (NoSuchMethodException e) {
+ resolvedAddresses = InetAddress.getAllByName(getHostString(address));
+ } catch (InvocationTargetException e) {
+ resolvedAddresses = InetAddress.getAllByName(getHostString(address));
+ }
--- End diff --
That's possibly another issue with this way of exposing `getHostString, the presence of a security manager could prevent us from doing it as expected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by tophei <gi...@git.apache.org>.
Github user tophei commented on the issue:
https://github.com/apache/zookeeper/pull/150
@anmolnar We have patched 451 into 3.4.12, and it worked in our setup so far. Thanks for the advise.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/150
Given the insights from the Kafka and K8s folks this looks like a good one to focus on.
@fpj any chance you can update this PR to address the conflicts?
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by ijuma <gi...@git.apache.org>.
Github user ijuma commented on the issue:
https://github.com/apache/zookeeper/pull/150
Thanks for pushing this useful improvement over the line @fpj.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by tophei <gi...@git.apache.org>.
Github user tophei commented on the issue:
https://github.com/apache/zookeeper/pull/150
@anmolnar Thanks for the comment. Actually I was trying to patch 451 into release tag 3.4.10 too , but seems it has much conflicts. By the way, may patch 451 have additional changes that may have related fix to above connection reading issue compared with patch 150?
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96170041
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
+ for (InetAddress resolvedAddress : resolvedAddresses) {
+ InetSocketAddress newAddr = new InetSocketAddress(resolvedAddress, thePort);
+ if (!serverAddresses.contains(newAddr)) {
+ serverAddresses.add(newAddr);
+ }
+ }
+ }
+ } catch (UnknownHostException e) {
+ LOG.warn("Cannot re-resolve server: " + curAddr + " UnknownHostException: " + e);
--- End diff --
Should we create an unresolved address like what we did on server side `recreateSocketAddresses` if we can't resolve the address? If the address is not resolvable but maybe it is still usable?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96574584
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
--- End diff --
OK, it is a reasonable concern of using package private API via reflection and I honestly don't know the implementation difference between Java 6 / 7 regarding getHostString - let's stick to explicit implementation in this file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by tophei <gi...@git.apache.org>.
Github user tophei commented on the issue:
https://github.com/apache/zookeeper/pull/150
We were recently run into this issue in cloud env.
Specifically, we have a kafka cluster setup in cloud, and it turns out to be a failure when the zookeeper node is replaced, namely the ip changed while the hostname remains same. We have patched this changes but it seems still not works. While the new zk node ip can be re-resolved and kafka can establish connection to zk node, but zk client seems not able to read content from the connection. Anyone encounters similar issue?
Pasted the log from kafka server.
[2018-05-11 03:47:46,806] WARN SASL configuration failed: javax.security.auth.login.LoginException: No JAAS configuration section named 'Client' was found in specified JAAS configuration file: '/usr/share/kafka/config/kafka_server_jaas.
conf'. Will continue connection to Zookeeper server without SASL authentication, if Zookeeper server allows it. (org.apache.zookeeper.ClientCnxn)
[2018-05-11 03:47:46,806] ERROR [ZooKeeperClient] Auth failed. (kafka.zookeeper.ZooKeeperClient)
[2018-05-11 03:47:46,806] ERROR [ZooKeeperClient] Auth failed. (kafka.zookeeper.ZooKeeperClient)
[2018-05-11 03:47:46,806] INFO Opening socket connection to server host.name/10.147.164.83:2181 (org.apache.zookeeper.ClientCnxn)
[2018-05-11 03:47:49,815] WARN Session 0x1634d42d88a001e for server null, unexpected error, closing socket connection and attempting reconnect (org.apache.zookeeper.ClientCnxn)
java.net.NoRouteToHostException: No route to host
at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method)
at sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717)
at org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1141)
[2018-05-11 03:47:50,667] INFO Resolving again hostname: host.name (org.apache.zookeeper.client.StaticHostProvider)
[2018-05-11 03:47:50,667] WARN Cannot re-resolve server: host.name/10.147.164.83:2181 (org.apache.zookeeper.client.StaticHostProvider)
java.net.UnknownHostException: host.name
at java.net.InetAddress.getAllByName0(InetAddress.java:1280)
at java.net.InetAddress.getAllByName(InetAddress.java:1192)
at java.net.InetAddress.getAllByName(InetAddress.java:1126)
at org.apache.zookeeper.client.StaticHostProvider.next(StaticHostProvider.java:142)
at org.apache.zookeeper.ClientCnxn$SendThread.startConnect(ClientCnxn.java:997)
at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1060)
[2018-05-11 03:47:51,668] WARN SASL configuration failed: javax.security.auth.login.LoginException: No JAAS configuration section named 'Client' was found in specified JAAS configuration file: '/usr/share/kafka/config/kafka_server_jaas.
conf'. Will continue connection to Zookeeper server without SASL authentication, if Zookeeper server allows it. (org.apache.zookeeper.ClientCnxn)
[2018-05-11 03:47:51,668] ERROR [ZooKeeperClient] Auth failed. (kafka.zookeeper.ZooKeeperClient)
[2018-05-11 03:47:51,668] ERROR [ZooKeeperClient] Auth failed. (kafka.zookeeper.ZooKeeperClient)
[2018-05-11 03:47:51,668] INFO Opening socket connection to server host.name/10.147.164.83:2181 (org.apache.zookeeper.ClientCnxn)
[2018-05-11 03:47:52,822] WARN Session 0x1634d42d88a001e for server null, unexpected error, closing socket connection and attempting reconnect (org.apache.zookeeper.ClientCnxn)
java.net.NoRouteToHostException: No route to host
at sun.nio.ch.SocketChannelImpl.checkConnect(Native Method)
at sun.nio.ch.SocketChannelImpl.finishConnect(SocketChannelImpl.java:717)
at org.apache.zookeeper.ClientCnxnSocketNIO.doTransport(ClientCnxnSocketNIO.java:361)
at org.apache.zookeeper.ClientCnxn$SendThread.run(ClientCnxn.java:1141)
[2018-05-11 03:47:53,427] INFO Resolving again hostname: host.name (org.apache.zookeeper.client.StaticHostProvider)
[2018-05-11 03:47:54,429] WARN SASL configuration failed: javax.security.auth.login.LoginException: No JAAS configuration section named 'Client' was found in specified JAAS configuration file: '/usr/share/kafka/config/kafka_server_jaas.
conf'. Will continue connection to Zookeeper server without SASL authentication, if Zookeeper server allows it. (org.apache.zookeeper.ClientCnxn)
[2018-05-11 03:47:54,429] ERROR [ZooKeeperClient] Auth failed. (kafka.zookeeper.ZooKeeperClient)
[2018-05-11 03:47:54,429] ERROR [ZooKeeperClient] Auth failed. (kafka.zookeeper.ZooKeeperClient)
[2018-05-11 03:47:54,429] INFO Opening socket connection to server host.name/10.147.161.203:2181 (org.apache.zookeeper.ClientCnxn)
[2018-05-11 03:47:55,383] INFO Socket connection established to host.name/10.147.161.203:2181, initiating session (org.apache.zookeeper.ClientCnxn)
[2018-05-11 03:47:55,386] INFO Unable to read additional data from server sessionid 0x1634d42d88a001e, likely server has closed socket, closing socket connection and attempting reconnect (org.apache.zookeeper.ClientCnxn)
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by glasser <gi...@git.apache.org>.
Github user glasser commented on the issue:
https://github.com/apache/zookeeper/pull/150
If you can provide a build of kafka with the fix in equivalent format to release distributions of Kafka I'd be happy to give it a shot.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96574629
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
+ for (InetAddress resolvedAddress : resolvedAddresses) {
+ InetSocketAddress newAddr = new InetSocketAddress(resolvedAddress, thePort);
+ if (!serverAddresses.contains(newAddr)) {
+ serverAddresses.add(newAddr);
+ }
+ }
+ }
+ } catch (UnknownHostException e) {
+ LOG.warn("Cannot re-resolve server: " + curAddr + " UnknownHostException: " + e);
--- End diff --
OK.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/150
We need get this moving, but let's first wait for feedback from @fpj before letting someone else taking over this JIRA.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by rcillo <gi...@git.apache.org>.
Github user rcillo commented on the issue:
https://github.com/apache/zookeeper/pull/150
This feature is highly valuable for the community. It could solve the problem of every team deploying Kafka on the cloud. Kafka has a static configuration with the IP addresses of Zookeeper nodes. If you need to replace these nodes and consequently change their IP addresses, you need to change Kafka configuratino file and then restart all Kafka nodes so that they will reload the updated configuration.
If this feature is merged, everyone deploying Kafka on the cloud could configure it using a load balancer address, that would be re-resolved from time to time, so that new Zookeeper instances would be automatically reachable from Kafka without the need of restarts. This would greatly improve the availability of Kafka.
Looking forward to have this merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96702291
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
- InetAddress ia = address.getAddress();
- InetAddress resolvedAddresses[] = InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
- address.getHostName());
+ InetAddress resolvedAddresses[];
+ try {
+ Method m = InetSocketAddress.class.getDeclaredMethod("getHostString");
+ m.setAccessible(true);
+ resolvedAddresses = InetAddress.getAllByName((String) m.invoke(address));
+ } catch (IllegalAccessException e) {
--- End diff --
Right, I forgot the context, thanks for pointing this out @eribeiro
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on the issue:
https://github.com/apache/zookeeper/pull/150
The error is expected because we haven't setup QA to build out of the 3.4 branch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by the-xs <gi...@git.apache.org>.
Github user the-xs commented on the issue:
https://github.com/apache/zookeeper/pull/150
Should we still wait or get this moving to fix the merge conflicts?
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96701932
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
- InetAddress ia = address.getAddress();
- InetAddress resolvedAddresses[] = InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
- address.getHostName());
+ InetAddress resolvedAddresses[];
+ try {
+ Method m = InetSocketAddress.class.getDeclaredMethod("getHostString");
+ m.setAccessible(true);
+ resolvedAddresses = InetAddress.getAllByName((String) m.invoke(address));
+ } catch (IllegalAccessException e) {
--- End diff --
Nope 'cause JDK6 here, no?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by sslavic <gi...@git.apache.org>.
Github user sslavic commented on the issue:
https://github.com/apache/zookeeper/pull/150
@riccardofreixo have you tried using ClusterIP Service for ZooKeeper StatefulSet and providing that ClusterIP (or service hostname) to Kafka / ZooKeeper clients as sole ZooKeeper hostname?
StatefulSet can have multiple replicas, but to ZooKeeper clients all of the members no matter how many of them there are (1, 3, 5, ..) would be accessible under single ClusterIP.
Even when Pods of StatefulSet die and get re-scheduled for whatever reason, they will likely get new IP, but IP of ClusterIP Service remains stable so ZooKeeper clients should be able to reconnect, without need to reresolve IP address of the host.
If there's a quorum, Pod that died does not necessarily have to become available quickly, clients should still be able to connect even without losing session.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96163440
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
--- End diff --
I failed to find any test case which covers the newly added condition, could you please point me to that. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96512625
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
--- End diff --
In the case where during the construction of the `StaticHostProvider` we add a host H1 which resolves to addresses A,B,C. Then at some point in the future A goes offline and the DNS system has changed H1 to resolve to hosts D,E,F. With the current code wouldn't `serverAddresses` now contain hosts B,C,D,E,F instead of only hosts D,E,F?
Is this something even worth fixing?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/150
@jeffwidman You're right. @fpj is able to close it.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96170084
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
--- End diff --
Java 6 has getHostString, but it's package private. Use reflection can access that. Maybe we should use the library version instead?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/150
@tophei @glasser This issue has become top priority quite recently, because more and more users reporting similar issues that you're facing with, so I'd like to get #451 in as soon as possible.
But we also have to be careful. #451 fixes it in a completely different way and I'd like to be confident about that the fix is right. I'll see if I can put together a Kafka build for you for testing.
@tophei Yes, #451 has a different approach. It should not have conflicts with 3.4.12 or you can just grab the latest from my branch and build it. Please drop me an email if you need help.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98564047
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,106 @@ public StaticHostProvider(Collection<InetSocketAddress> 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) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ String curHostString = getHostString(curAddr);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
--- End diff --
whoops, apologies for the duplicate. \U0001f627
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by edvorkin <gi...@git.apache.org>.
Github user edvorkin commented on the issue:
https://github.com/apache/zookeeper/pull/150
+1 one here. This feature is necessary for running Zookeeper in the cloud under AWS ASG. Every time node fails, ASG reassigns new IP for new zookeeper and there is no way kafka will know about it. We need to treat zookeeper servers as cattle, not pets, and kill and spin new one at will without affecting kafka.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by djenriquez <gi...@git.apache.org>.
Github user djenriquez commented on the issue:
https://github.com/apache/zookeeper/pull/150
Hi guys, any update for this PR? Many thanks!!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by nicorevin <gi...@git.apache.org>.
Github user nicorevin commented on the issue:
https://github.com/apache/zookeeper/pull/150
@fpj any updates on it? It seems like a blocker for clustering kafka (and everyone using zkclient) in docker/kubernetes.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98263678
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,104 @@ public StaticHostProvider(Collection<InetSocketAddress> 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;
+
+ 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 (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
+ LOG.info("Resolving again hostname: {}", getHostString(curAddr));
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ nextAdded = 0;
+ nextRemoved = 0;
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ nextAdded = nextRemoved = 1;
+ LOG.debug("Newly resolved address: {}", resolvedAddresses[0]);
+ } else {
+ int i = 0;
+ while(i < serverAddresses.size()) {
+ if(getHostString(serverAddresses.get(i)) == getHostString(curAddr)) {
--- End diff --
Ops, are we comparing strings with `==` ?! ;)
Nit: space between `if` and (`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by edwardoliveira <gi...@git.apache.org>.
Github user edwardoliveira commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98263409
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,104 @@ public StaticHostProvider(Collection<InetSocketAddress> 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;
+
+ 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 (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
+ LOG.info("Resolving again hostname: {}", getHostString(curAddr));
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ nextAdded = 0;
+ nextRemoved = 0;
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ nextAdded = nextRemoved = 1;
+ LOG.debug("Newly resolved address: {}", resolvedAddresses[0]);
+ } else {
+ int i = 0;
+ while(i < serverAddresses.size()) {
+ if(getHostString(serverAddresses.get(i)) == getHostString(curAddr)) {
--- End diff --
Ops, are we comparing strings with `==` ?! ;)
Nit: space between `if` and `(`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96431605
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,12 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
--- End diff --
I've added a phrase to the return tag.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96699514
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
- InetAddress ia = address.getAddress();
- InetAddress resolvedAddresses[] = InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
- address.getHostName());
+ InetAddress resolvedAddresses[];
+ try {
+ Method m = InetSocketAddress.class.getDeclaredMethod("getHostString");
+ m.setAccessible(true);
+ resolvedAddresses = InetAddress.getAllByName((String) m.invoke(address));
+ } catch (IllegalAccessException e) {
--- End diff --
Maybe we can catch multiple exceptions in a single shot - such as `catch(IllegalAccessException | NoSuchMethodException | InvocationTargetException e)` to save some typings, given the exception handling logic is exact the same.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98535935
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,106 @@ public StaticHostProvider(Collection<InetSocketAddress> 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) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ String curHostString = getHostString(curAddr);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
--- End diff --
nit: why not use curHostString instead of calling getHostString two more times?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96509432
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
--- End diff --
instead of `null` checking `ia` couldn't we use `addr.isUnresolved()`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/150
Thanks everyone for the great effort & time in pushing this issue. I could see long discussions in the PR and I hope this work is nearing completion. Could you please update the progress and would like to know the chances of pushing this asap, thanks!.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96509843
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,12 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
--- End diff --
nit: Can we swap the first two lines of this comment, as the reader does not know what class getHostString belongs to until he/she reaches line 2?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by riccardofreixo <gi...@git.apache.org>.
Github user riccardofreixo commented on the issue:
https://github.com/apache/zookeeper/pull/150
@sslavic thanks for the suggestion.
We haven't tried that approach, and as far as I can tell it sounds like it would work. You'd still have the re-resolution problem if you deleted/recreated the service, but that should be quite rare. Had we thought of that before, we probably wouldn't have patched the client. Now we have though, we'll keep it patched.
I still think this should be fixed on the zk-client, as there are other circumstances other than Kube where the IP addresses may change and you wouldn't have an easy solution such as ClusterIP.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96226819
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -45,6 +45,9 @@
private int currentIndex = -1;
+ // Don't re-resolve on first next() call
+ private boolean connectedSinceNext = true;
--- End diff --
My reasoning was only defensively, but on a 2nd look, it would be overkill. So, no problem without being volatile, imo.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/150
@phunt @afine @fpj I'm happy to pick this up tomorrow.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by edwardoliveira <gi...@git.apache.org>.
Github user edwardoliveira commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98483994
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -122,18 +122,19 @@ public int size() {
private int nextAdded = 0;
private int nextRemoved = 0;
- public int getNextAdded() {
+ int getNextAdded() {
return nextAdded;
}
- public int getNextRemoved() {
+ int getNextRemoved() {
return nextRemoved;
}
public InetSocketAddress next(long spinDelay) {
// Handle possible connection error by re-resolving hostname if possible
if (!connectedSinceNext) {
InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ String curHostString = getHostString(curAddr);
if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
--- End diff --
@fpj Sorry for yet another comment, mate (my last one, I promise). I cited this previously but certainly got lost in my verbosite.
* Replace `if (!getHostString(currAddr).equals(...)) {` by 'if (!curHostString.equals()` at Line 138;
* Replace `getHostString(curAddr)` with `curHostString` at line 139;
* Replace `getHostString(curAddr)` with `curHostString` at line 142;
**+1. LGTM. Really great job!.**
Best regards,
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98280911
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,104 @@ public StaticHostProvider(Collection<InetSocketAddress> 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;
+
+ 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 (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
+ LOG.info("Resolving again hostname: {}", getHostString(curAddr));
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ nextAdded = 0;
+ nextRemoved = 0;
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ nextAdded = nextRemoved = 1;
+ LOG.debug("Newly resolved address: {}", resolvedAddresses[0]);
+ } else {
+ int i = 0;
+ while(i < serverAddresses.size()) {
+ if(getHostString(serverAddresses.get(i)) == getHostString(curAddr)) {
--- End diff --
Why call `getHostString(curAddr)` (here in a loop, at line 137, 138, 141) if the `currAddr` doesn't change? Better call once between line 136 and 137 and assign to a variable, no?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98392957
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,104 @@ public StaticHostProvider(Collection<InetSocketAddress> 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;
+
+ 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 (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
+ LOG.info("Resolving again hostname: {}", getHostString(curAddr));
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ nextAdded = 0;
+ nextRemoved = 0;
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ nextAdded = nextRemoved = 1;
+ LOG.debug("Newly resolved address: {}", resolvedAddresses[0]);
+ } else {
+ int i = 0;
--- End diff --
You right.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96699622
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
- InetAddress ia = address.getAddress();
- InetAddress resolvedAddresses[] = InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
- address.getHostName());
+ InetAddress resolvedAddresses[];
+ try {
+ Method m = InetSocketAddress.class.getDeclaredMethod("getHostString");
+ m.setAccessible(true);
+ resolvedAddresses = InetAddress.getAllByName((String) m.invoke(address));
+ } catch (IllegalAccessException e) {
+ resolvedAddresses = InetAddress.getAllByName(getHostString(address));
+ } catch (NoSuchMethodException e) {
+ resolvedAddresses = InetAddress.getAllByName(getHostString(address));
+ } catch (InvocationTargetException e) {
+ resolvedAddresses = InetAddress.getAllByName(getHostString(address));
+ }
--- End diff --
The signature of getDeclaredMethod said it also could throw SecurityException, not sure if we should catch it or not here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98281621
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,104 @@ public StaticHostProvider(Collection<InetSocketAddress> 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;
+
+ 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 (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
+ LOG.info("Resolving again hostname: {}", getHostString(curAddr));
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ nextAdded = 0;
+ nextRemoved = 0;
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ nextAdded = nextRemoved = 1;
+ LOG.debug("Newly resolved address: {}", resolvedAddresses[0]);
+ } else {
+ int i = 0;
--- End diff --
silly refactoring:
```
List<String> toRemove = new ArrayList<>(serverAddresses.size());
for (String addr : serverAddresses) {
if (getHostString(addr).equals(hostString) {
toRemove.add(addr);
}
}
LOG.debug("Removing addresses: {}", toRemove);
nextRemoved += toRemove.size();
serverAddresses.removeAll(toRemove);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98263780
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,104 @@ public StaticHostProvider(Collection<InetSocketAddress> 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) {
--- End diff --
nit: space between `if` and `(`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96120356
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,12 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if ( ia != null ) {
+ // 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.indexOf( ':' ));
+ }
+
--- End diff --
Extra space?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r97437693
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +86,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
+ for (InetAddress resolvedAddress : resolvedAddresses) {
+ InetSocketAddress newAddr = new InetSocketAddress(resolvedAddress, thePort);
+ if (!serverAddresses.contains(newAddr)) {
+ serverAddresses.add(newAddr);
+ }
+ }
--- End diff --
We shuffle initially to avoid having all clients connecting to the same server in the case they are all given the same connect string. If the array of addresses has already been shuffled (in the constructor), then the order followed in this method will be the shuffled one. I don't see a strong reason for re-shuffling, as we are not bringing it back to the original order by resolving again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by jeffwidman <gi...@git.apache.org>.
Github user jeffwidman commented on the issue:
https://github.com/apache/zookeeper/pull/150
Any movement on this?
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96642443
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
- InetAddress ia = address.getAddress();
- InetAddress resolvedAddresses[] = InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
- address.getHostName());
+ InetAddress resolvedAddresses[];
+ try {
--- End diff --
@hanm have a look at this, please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by bwmills <gi...@git.apache.org>.
Github user bwmills commented on the issue:
https://github.com/apache/zookeeper/pull/150
As noted by @rcillo back in June - this feature is highly valuable for the community. It's certainly of critical importance to our production services in K8s. Any updates are much appreciated.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/150
This PR has to be rebased first before it can be merged. Hi Flavio - will you follow up with this or you prefer someone else take this over? @fpj @rakeshadr
I've updated the JIRA to mark it as a blocker for next release (3.4.11, 3.5.4), to prevent this issue lagging again. Good to get this in given its impact and relatively little effort given the PR is already in a good shape.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/150
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2461/
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98543324
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,106 @@ public StaticHostProvider(Collection<InetSocketAddress> 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) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ String curHostString = getHostString(curAddr);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
+ LOG.info("Resolving again hostname: {}", getHostString(curAddr));
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ nextAdded = 0;
+ nextRemoved = 0;
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ nextAdded = nextRemoved = 1;
+ LOG.debug("Newly resolved address: {}", resolvedAddresses[0]);
+ } else {
+ int i = 0;
+ while (i < serverAddresses.size()) {
+ if (getHostString(serverAddresses.get(i)).equals(curHostString) &&
--- End diff --
I think that this fixes the issue I described.
nit: Just wondering if it would be easier to use a map from (hoststring,port) -> serverAddress to make things clearer/remove a loop?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96694626
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
--- End diff --
Yup, we could: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b27/java/net/InetSocketAddress.java#258
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98265031
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,104 @@ public StaticHostProvider(Collection<InetSocketAddress> 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;
+
+ public int getNextAdded() {
--- End diff --
okay, just me being pedant here, but it's a sincere question: would it be *worth* to move this class to `src/java/test/org/apache/zookeeper/client/StaticHostProvider.java` and then remove the `public` modifier so that `getNextAdded()` and `getNextRemoved()` can be package protected as they are used for tests?
At first, I think it doesn't see worth this kind of change, but I am uncomfortable with leaving those methods as public if they are used for tests by now. :thinking:
Wdyt?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96120333
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,12 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if ( ia != null ) {
--- End diff --
Silly nit: space around expression.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96125997
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -45,6 +45,9 @@
private int currentIndex = -1;
+ // Don't re-resolve on first next() call
+ private boolean connectedSinceNext = true;
--- End diff --
All calls to `next` and `onConnected` are from the `sendThread`. I don't see a reason for making volatile, unless we are doing it defensively. Let me know if I'm missing anything.
Note that this pull request is for the 3.4 branch, we need a different patch for 3.5 and master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by tophei <gi...@git.apache.org>.
Github user tophei commented on the issue:
https://github.com/apache/zookeeper/pull/150
@glasser Thanks for the sharing. Yeah, a static ip indeed would help work around the issue. However, assign static ip to service in k8s seems to be a bit heavy and less flexible when you need to setup massive of clusters.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by riccardofreixo <gi...@git.apache.org>.
Github user riccardofreixo commented on the issue:
https://github.com/apache/zookeeper/pull/150
We're running Kafka in Kubernetes, so this bug was biting us regularly.
We applied the patch in the kafka clusters of our client and are running in prod. Solves our problem and created no additional problems for us.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96453176
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
+ for (InetAddress resolvedAddress : resolvedAddresses) {
+ InetSocketAddress newAddr = new InetSocketAddress(resolvedAddress, thePort);
+ if (!serverAddresses.contains(newAddr)) {
+ serverAddresses.add(newAddr);
+ }
+ }
+ }
+ } catch (UnknownHostException e) {
+ LOG.warn("Cannot re-resolve server: " + curAddr + " UnknownHostException: " + e);
--- End diff --
https://www.slf4j.org/api/org/slf4j/Logger.html#warn(org.slf4j.Marker, java.lang.String, java.lang.Throwable)
Can we do like,
LOG.warn("Cannot re-resolve server: {}, exception: ", curAddr, e);
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by glasser <gi...@git.apache.org>.
Github user glasser commented on the issue:
https://github.com/apache/zookeeper/pull/150
@tophei yes, it's pretty bad. We eventually (in k8s) set up a service per zk pod (giving them effectively static IPs) to deal with this.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96442215
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
+ for (InetAddress resolvedAddress : resolvedAddresses) {
+ InetSocketAddress newAddr = new InetSocketAddress(resolvedAddress, thePort);
+ if (!serverAddresses.contains(newAddr)) {
+ serverAddresses.add(newAddr);
+ }
+ }
+ }
+ } catch (UnknownHostException e) {
+ LOG.warn("Cannot re-resolve server: " + curAddr + " UnknownHostException: " + e);
--- End diff --
If the `StaticHostProvider` constructor didn't throw an `UnknownHostException`, then I'd think that all names and addresses we have are good. I'm not sure what could cause an `UnknownHostException` in `next()` other than some transient error. If that's right, then I'm not sure we should be adding or removing anything.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96227005
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
+ for (InetAddress resolvedAddress : resolvedAddresses) {
+ InetSocketAddress newAddr = new InetSocketAddress(resolvedAddress, thePort);
+ if (!serverAddresses.contains(newAddr)) {
+ serverAddresses.add(newAddr);
+ }
+ }
+ }
+ } catch (UnknownHostException e) {
+ LOG.warn("Cannot re-resolve server: " + curAddr + " UnknownHostException: " + e);
--- End diff --
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/150
@tophei Sounds great, thanks for testing.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98484702
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -122,18 +122,19 @@ public int size() {
private int nextAdded = 0;
private int nextRemoved = 0;
- public int getNextAdded() {
+ int getNextAdded() {
return nextAdded;
}
- public int getNextRemoved() {
+ int getNextRemoved() {
return nextRemoved;
}
public InetSocketAddress next(long spinDelay) {
// Handle possible connection error by re-resolving hostname if possible
if (!connectedSinceNext) {
InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ String curHostString = getHostString(curAddr);
--- End diff --
edwardoliveira just now
@fpj Sorry for yet another comment, mate (my last one, I promise). I cited this previously but certainly got lost in my verbosite.
Replace if (!getHostString(currAddr).equals(...)) { by 'if (!curHostString.equals()` at Line 138;
Replace getHostString(curAddr) with curHostString at line 139;
Replace getHostString(curAddr) with curHostString at line 142;
**IMHO, +1. Really great job!** :+1:
Best regards,
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by jorgheymans <gi...@git.apache.org>.
Github user jorgheymans commented on the issue:
https://github.com/apache/zookeeper/pull/150
just got stung by this as well, assumed zk clients would be clever enough to reresolve :-/
Since there is a lot of interest in this why not just rebase-merge and let ppl test out the snapshot builds ?
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96439377
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
--- End diff --
We can do it, but I'm not super convinced we should because we are essentially using a method with undocumented API. Perhaps it does the same as the one in Java 7, with the difference that it is public, but I'm worried that there could be some correctness issue involved. Do you know more about it?
In any case, I'm going to push the changes so that we see how it looks like.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/150
Does C client has similar issue?
Also, ZOOKEEPER-2184 looks like a dup of ZOOKEEPER-338.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96433850
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
+ for (InetAddress resolvedAddress : resolvedAddresses) {
+ InetSocketAddress newAddr = new InetSocketAddress(resolvedAddress, thePort);
+ if (!serverAddresses.contains(newAddr)) {
+ serverAddresses.add(newAddr);
+ }
+ }
+ }
+ } catch (UnknownHostException e) {
+ LOG.warn("Cannot re-resolve server: " + curAddr + " UnknownHostException: " + e);
--- End diff --
I don't think I can do both, see the API docs of slf4j:
https://www.slf4j.org/api/org/slf4j/Logger.html
In the case you are, tell me which one is your favorite. I'd say the curly braces.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98385538
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,104 @@ public StaticHostProvider(Collection<InetSocketAddress> 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;
+
+ 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 (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
+ LOG.info("Resolving again hostname: {}", getHostString(curAddr));
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ nextAdded = 0;
+ nextRemoved = 0;
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ nextAdded = nextRemoved = 1;
+ LOG.debug("Newly resolved address: {}", resolvedAddresses[0]);
+ } else {
+ int i = 0;
--- End diff --
Although this is more Java-like, it requires the creation of an additional ArrayList, which is less efficient than creating a int counter. Unless there is something wrong with the current code, I'd rather leave as is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96120303
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,12 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
--- End diff --
This line could be part of the @return tag, no?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96699234
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
- InetAddress ia = address.getAddress();
- InetAddress resolvedAddresses[] = InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
- address.getHostName());
+ InetAddress resolvedAddresses[];
+ try {
--- End diff --
It might be better to wrap the reflection in an abstraction in a static block in this file so inspection of the class will be done only once, save some runtime inspection cycles.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98549400
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,106 @@ public StaticHostProvider(Collection<InetSocketAddress> 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) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ String curHostString = getHostString(curAddr);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
--- End diff --
@afine https://github.com/apache/zookeeper/pull/150#discussion_r98484702 :sunglasses:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r97436370
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
--- End diff --
OK.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98565216
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,106 @@ public StaticHostProvider(Collection<InetSocketAddress> 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) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ String curHostString = getHostString(curAddr);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
--- End diff --
No problem at all. \U0001f603
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96163423
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
+ for (InetAddress resolvedAddress : resolvedAddresses) {
+ InetSocketAddress newAddr = new InetSocketAddress(resolvedAddress, thePort);
+ if (!serverAddresses.contains(newAddr)) {
+ serverAddresses.add(newAddr);
+ }
+ }
+ }
+ } catch (UnknownHostException e) {
+ LOG.warn("Cannot re-resolve server: " + curAddr + " UnknownHostException: " + e);
--- End diff --
Please use {} instead of string concatenation. Also, for better debugging, can we pass 'e' object as argument to the logger instead of concat the exception message.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by jeffwidman <gi...@git.apache.org>.
Github user jeffwidman commented on the issue:
https://github.com/apache/zookeeper/pull/150
IIUC, this PR should be closed in favor of #451, which has already been merged.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96121135
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,12 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if ( ia != null ) {
+ // 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.indexOf( ':' ));
+ }
+
+ return hostString;
+ }
+
public int size() {
return serverAddresses.size();
}
public InetSocketAddress next(long spinDelay) {
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
+ for (InetAddress resolvedAddress : resolvedAddresses) {
+ InetSocketAddress newAddr = new InetSocketAddress(resolvedAddress, thePort);
+ if (!serverAddresses.contains(newAddr)) {
+ serverAddresses.add(newAddr);
+ }
+ }
+ }
+ } catch (UnknownHostException e) {
+ LOG.warn("Cannot re-resolve server: " + curAddr + " UnknownHostException: " + e);
+ }
+ }
+ }
++currentIndex;
+ connectedSinceNext = false;
if (currentIndex == serverAddresses.size()) {
--- End diff --
As `serverAddresses.size()` cannot be 0 (per constructor checking) this if condition and line 137 could be rewritten as:
``
currentIndex = currentIndex % serverAddresses.size();
``
or even
``
currentIndex = ++currentIndex % serverAddresses.size();
``
Eliminating the need of line 137 too.
On master, branch-3.4 and branch-3.5. **Just a silly optimization, tough.**
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r97446760
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +73,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
--- End diff --
@afine check the new changes to see if they address this and make sense.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r97435427
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
- InetAddress ia = address.getAddress();
- InetAddress resolvedAddresses[] = InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
- address.getHostName());
+ InetAddress resolvedAddresses[];
+ try {
--- End diff --
I'm still not sure we should do this. I'm concerned about making that method visible while the original intention was not to expose it. Are you aware of any other project that has done this for `getHostString`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r98282550
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +75,104 @@ public StaticHostProvider(Collection<InetSocketAddress> 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;
+
+ 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 (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!getHostString(curAddr).equals(curAddr.getAddress().getHostAddress())) {
+ LOG.info("Resolving again hostname: {}", getHostString(curAddr));
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ nextAdded = 0;
+ nextRemoved = 0;
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ nextAdded = nextRemoved = 1;
+ LOG.debug("Newly resolved address: {}", resolvedAddresses[0]);
+ } else {
+ int i = 0;
--- End diff --
```
Line 136: String hostString = getHostString(currAddr);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/150
I suspect folks were out on vacation. I was. :-)
It doesn't seem like @fpj has time to look at this - can someone else pick it up and address the recent comments?
---
[GitHub] zookeeper issue #150: ZOOKEEPER-2184: Zookeeper Client should re-resolve hos...
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on the issue:
https://github.com/apache/zookeeper/pull/150
@hanm I believe we do have the same issue with the C client, I don't see it re-resolving addresses there, I need to have a closer look, though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r97921838
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -57,26 +62,20 @@
public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
throws UnknownHostException {
for (InetSocketAddress address : serverAddresses) {
- InetAddress ia = address.getAddress();
- InetAddress resolvedAddresses[] = InetAddress.getAllByName((ia!=null) ? ia.getHostAddress():
- address.getHostName());
+ InetAddress resolvedAddresses[];
+ try {
--- End diff --
@fpj Let's not do this (expose package private JDK methods) given the concerns you raised earlier (as I also commented previously).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #150: ZOOKEEPER-2184: Zookeeper Client should re-reso...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/150#discussion_r96696184
--- Diff: src/java/main/org/apache/zookeeper/client/StaticHostProvider.java ---
@@ -87,15 +86,69 @@ public StaticHostProvider(Collection<InetSocketAddress> serverAddresses)
Collections.shuffle(this.serverAddresses);
}
+ /**
+ * 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().
+ *
+ * It evaluates to a hostname if one is available and otherwise it returns the
+ * string representation of the IP address.
+ *
+ * @param addr
+ * @return Hostname string of address parameter
+ */
+ private String getHostString(InetSocketAddress addr) {
+ String hostString;
+ InetAddress ia = addr.getAddress();
+
+ if (ia != null) {
+ // 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();
}
public InetSocketAddress next(long spinDelay) {
- ++currentIndex;
- if (currentIndex == serverAddresses.size()) {
- currentIndex = 0;
+ // Handle possible connection error by re-resolving hostname if possible
+ if (!connectedSinceNext) {
+ InetSocketAddress curAddr = serverAddresses.get(currentIndex);
+ if (!curAddr.getHostString().equals(curAddr.getAddress().getHostAddress())) {
+ try {
+ int thePort = curAddr.getPort();
+ InetAddress resolvedAddresses[] = InetAddress.getAllByName(getHostString(curAddr));
+ if (resolvedAddresses.length == 1) {
+ serverAddresses.set(currentIndex, new InetSocketAddress(resolvedAddresses[0], thePort));
+ } else {
+ serverAddresses.remove(currentIndex);
+ for (InetAddress resolvedAddress : resolvedAddresses) {
+ InetSocketAddress newAddr = new InetSocketAddress(resolvedAddress, thePort);
+ if (!serverAddresses.contains(newAddr)) {
+ serverAddresses.add(newAddr);
+ }
+ }
--- End diff --
As we are adding new addresses into `serverAddresses`, would it make sense to re-shuffle the list?
```
Collections.shuffle(this.serverAddresses);
```
As we did in constructor???
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---