You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by JiangJiafu <gi...@git.apache.org> on 2017/02/13 11:51:18 UTC
[GitHub] zookeeper pull request #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
GitHub user JiangJiafu opened a pull request:
https://github.com/apache/zookeeper/pull/173
ZOOKEEPER-2691: recreateSocketAddresses may recreate the unreachable \u2026
\u2026IP address
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/JiangJiafu/zookeeper ZOOKEEPER-2691
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/173.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 #173
----
commit 489cec9b78b21a2a241eeab18ddfb968758b2e67
Author: JiangJiafu <ji...@gmail.com>
Date: 2017-02-13T11:36:44Z
ZOOKEEPER-2691: recreateSocketAddresses may recreate the unreachable IP address
----
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/173
nit: would this change be worth documenting?
Otherwise, +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 pull request #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r101843611
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -194,6 +237,12 @@ public void recreateSocketAddresses() {
public long id;
public LearnerType type = LearnerType.PARTICIPANT;
+
+ /**
+ * the time, in millseconds, before {@link InetAddress#isReachable} aborts
--- End diff --
typo: miliseconds.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r100940145
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -159,7 +159,7 @@ public QuorumServer(long id, String hostname,
public void recreateSocketAddresses() {
InetAddress address = null;
try {
- address = InetAddress.getByName(this.hostname);
+ address = getReachableAddress(this.hostname, 2000);
--- End diff --
Thanks for your advices, I will send another pull request later.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r101843694
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -128,24 +136,32 @@ private QuorumServer(long id, InetSocketAddress addr,
this.addr = addr;
this.electionAddr = electionAddr;
this.type = type;
+ String checkIPReachableValue = System.getProperty("zookeeper.checkIPTimeout");
+ if(checkIPReachableValue != null){
+ this.checkIPReachableTO = Integer.parseInt(checkIPReachableValue);
+ }
}
public QuorumServer(long id, String hostname,
Integer port, Integer electionPort,
LearnerType type) {
- this.id = id;
- this.hostname=hostname;
- if (port!=null){
+ this.id = id;
+ this.hostname=hostname;
+ if (port!=null){
this.port=port;
- }
- if (electionPort!=null){
+ }
+ if (electionPort!=null){
this.electionPort=electionPort;
- }
- if (type!=null){
+ }
+ if (type!=null){
this.type = type;
- }
- this.recreateSocketAddresses();
}
+ String checkIPReachableValue = System.getProperty("zookeeper.checkIPTimeout");
+ if(checkIPReachableValue != 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 issue #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on the issue:
https://github.com/apache/zookeeper/pull/173
@hanm Yes, I hope this PR can be solved in 3.4.10~ Since 3.5.X is still in alpha state now.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r116099795
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -181,6 +197,33 @@ public void recreateSocketAddresses() {
}
}
+ /**
+ * Resolve the hostname to IP addresses, and find one reachable address.
+ *
+ * @param hostname the name of the host
+ * @param timeout the time, in millseconds, before {@link InetAddress#isReachable}
+ * aborts
+ * @return a reachable IP address. If no such IP address can be found,
+ * just return the first IP address of the hostname.
+ *
+ * @exception UnknownHostException
+ */
+ public InetAddress getReachableAddress(String hostname, int timeout)
+ throws UnknownHostException {
+ InetAddress[] addresses = InetAddress.getAllByName(hostname);
+ for (InetAddress a : addresses) {
+ try {
+ if (a.isReachable(timeout)) {
--- End diff --
I think this is a valid concern. On top of this, I think we should make sure user can resort to old behavior if needed. With this patch the `isReachable` will be called in any case, regardless of the property 'zookeeper.ipReachableTimeout' is defined or not. How about something like this:
if (zookeeper.ipReachableTimeout is not defined) {
address = InetAddress.getByName(this.hostname);
} else {
address = getReachableAddress(this.hostname, ipReachableTimeout);
}
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on the issue:
https://github.com/apache/zookeeper/pull/173
@rakeshadr Thank you for your respond.
If you have decided how to fix this problem, please let me know.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/173
Thanks for updates, @JiangJiafu. Patch looks good to me, will merge soon.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by bensherman <gi...@git.apache.org>.
Github user bensherman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r120952370
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1076,6 +1076,26 @@ server.3=zoo3:2888:3888</programlisting>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>zookeeper.ipReachableTimeout</term>
--- End diff --
Looks like there is a small problem with this as I add my change to documentation.
Shouldn't this say
` <term>ipReachableTimeout</term>
`
zookeeper is added by the system property setter, but isn't required (and won't work) if it is actually in the config 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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/173
I'm really sorry, I didn't get much time on this work. I could see very good discussions about this and I'm thinking you all are reaching to a pretty good solution. My previous comment was to converge the 3.4.10 release discussions and filter out tasks if it is not really important now.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on the issue:
https://github.com/apache/zookeeper/pull/173
@hanm Hi, May I ask when will this problem be fixed? And will it be fixed on 3.4.X(stable) version? 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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/173
@jiangjiafu glad to help. :) Thanks for driving this change. Great work.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r116798516
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -198,6 +246,12 @@ public void recreateSocketAddresses() {
public long id;
public LearnerType type = LearnerType.PARTICIPANT;
+
+ /**
+ * the time, in milliseconds, before {@link InetAddress#isReachable} aborts
+ * in {@link #getReachableAddress}.
+ */
+ private int ipReachableTimeout = 0;
--- End diff --
I think we can remove this. This will also remove the four copies of exact same initialization code fragment that you use to initialize the value. We can get the sys property and parse the value inside recreateSocketAddress directly.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/173
>> on the surface this pr reminds me of ZOOKEEPER-2184, wondering if some of the logic can be shared?
I had a chat with @afine offline again about this and I think Abe had a good point of view of both problems at a higher level - they are not resolving exact same problem but there is an opportunity that we could do some refactoring and code sharing regarding effectively map between usable IP and host names. This improvement could be done later though given the current many things in flight status to reduce the risk of this PR being stale...
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on the issue:
https://github.com/apache/zookeeper/pull/173
@eribeiro Thank you for your review.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/173
Thanks for updates, @JiangJiafu.
Patch looks good, and I agree that this patch is solving a different problem than ZOOKEEPER-2184. I'll give a few more days in case anyone has addition feedback before merging this in.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/173
Thanks a lot @JiangJiafu for the effort in driving this.
I'm not sure whether this is a blocker for 3.4.10 release. Is it good if we just release 3.4.10 without this and make it a blocker for 3.4.11 and 3.5.3-alpha? Also, we could set a time frame for 3.5.3-alpha release and mark this to beta version, later.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r123022889
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1076,6 +1076,26 @@ server.3=zoo3:2888:3888</programlisting>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>zookeeper.ipReachableTimeout</term>
--- End diff --
Thanks @bensherman for pointing this out. Fixed in commit 677b5ccb7ce214eee750fda70eba3643d8c7a6c0.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r101843810
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -128,24 +136,32 @@ private QuorumServer(long id, InetSocketAddress addr,
this.addr = addr;
this.electionAddr = electionAddr;
this.type = type;
+ String checkIPReachableValue = System.getProperty("zookeeper.checkIPTimeout");
+ if(checkIPReachableValue != 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 issue #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/173
I'll review later, but a quick answer to your question on how to update doc: no don't modify the html directly. Instead only modify the source of the docs. The sources are in folder src/docs/src/documentation/content/xdocs . In addition it would be good to verify your doc change locally by compiling the doc source with apache forrest (https://forrest.apache.org/). But please don't include the compiled documents (the html and pdf files) as part of the patch - you only need to change the source of the documents. You can check commit history of src/docs/src/documentation/content/xdocs and learn by example, should be pretty straightforward..
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu closed the pull request at:
https://github.com/apache/zookeeper/pull/173
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on the issue:
https://github.com/apache/zookeeper/pull/173
Please have a look to the new code, thank you.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r100893741
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -159,7 +159,7 @@ public QuorumServer(long id, String hostname,
public void recreateSocketAddresses() {
InetAddress address = null;
try {
- address = InetAddress.getByName(this.hostname);
+ address = getReachableAddress(this.hostname, 2000);
--- End diff --
Instead of hardcoding 2000 here, we can have a constant variable represent the timeout value at the beginning of QuorumPeer.java. [QuorumCnxManager](https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java) is a good example on how it uses constants.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on the issue:
https://github.com/apache/zookeeper/pull/173
@hanm Hi, I have modified the code according to your advices except the second one:
"Documentation (see Abe's comment)"
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on the issue:
https://github.com/apache/zookeeper/pull/173
Hi, @hanm , do you mean that you will merge ZOOKEEPER-2691 and ZOOKEEPER-2184 to solve the problem later? If so, may I ask will the problem be solved in version zokkeper 3.4.10?
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r101846854
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -113,13 +113,21 @@ private QuorumServer(long id, InetSocketAddress addr,
this.id = id;
this.addr = addr;
this.electionAddr = electionAddr;
+ String checkIPReachableValue = System.getProperty("zookeeper.checkIPTimeout");
--- End diff --
I think we can improve this naming game just a little bit. Why not call this variable `ipTimeout`? And `checkIPReachableTO` as `ipReachableTimeout` or `reachableIPTimeout`?
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r101845264
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -181,6 +197,33 @@ public void recreateSocketAddresses() {
}
}
+ /**
+ * Resolve the hostname to IP addresses, and find one reachable address.
+ *
+ * @param hostname the name of the host
+ * @param timeout the time, in millseconds, before {@link InetAddress#isReachable}
+ * aborts
+ * @return a reachable IP address. If no such IP address can be found,
+ * just return the first IP address of the hostname.
+ *
+ * @exception UnknownHostException
+ */
+ public InetAddress getReachableAddress(String hostname, int timeout)
+ throws UnknownHostException {
+ InetAddress[] addresses = InetAddress.getAllByName(hostname);
+ for (InetAddress a : addresses) {
+ try {
+ if (a.isReachable(timeout)) {
--- End diff --
My main problem with this PR is that call to `isReachable(timeout)` for two reasons:
1) the most important one: `isReachable(timeout)` seems unreliable so there are plenty cases where it returns false even tough the node is reachable or vice-versa! https://bugs.openjdk.java.net/browse/JDK-8159410 (google "InetAddress.isReachable not working" or "InetAddress.isReachable unreliable" to see further cases).
2) This timeout can add an arbitrary delay until a reachable node can be tested.
IDK what a good compromise would be for both points above (leaving as it is today could work, so no problem, even tough I am a bit concerned), but maybe we could use a solution similar to ZOOKEEPER-2184 and return the next address in the array (using `next = ++next % addresses.length` to prevent out of bound exceptions).
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on the issue:
https://github.com/apache/zookeeper/pull/173
@afine , I have tried a sample solution, which should modify only one ZooKeeper source file(if you don't need to pass all the original unittest). You can review the change here: https://github.com/apache/zookeeper/pull/173/files
I have read the ZOOKEEPER-2184, and I don't think we meet the exactly same problem, because it seems like that ZooKeeper client related code should be modified to fix the ZOOKEEPER-2184, but only ZooKeeper server side code should be modifiied to fix this problem.
By the way, ZOOKEEPER-2184 problem is fixed in version 3.5.3, 3.4.11 accroding to the jira record.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on the issue:
https://github.com/apache/zookeeper/pull/173
@hanm I have changed the document, please review the code and document, thank you.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on the issue:
https://github.com/apache/zookeeper/pull/173
Eh, sorry for asking this question, but how to update the documentation? Should I modify the html files in docs directory?
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/173
@JiangJiafu @rakeshadr @hanm @fpj Guys, does my considerations on this comment https://github.com/apache/zookeeper/pull/173#discussion_r101845264 make any sense? Probably it's just me being paranoid and/or playing devil's advocate, but I would love to hear your opinions.
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by JiangJiafu <gi...@git.apache.org>.
Github user JiangJiafu commented on the issue:
https://github.com/apache/zookeeper/pull/173
I have modified the source code according to your advise, please review the code. @hanm
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/173
PR merged, please close this @JiangJiafu
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r101843855
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -113,13 +113,21 @@ private QuorumServer(long id, InetSocketAddress addr,
this.id = id;
this.addr = addr;
this.electionAddr = electionAddr;
+ String checkIPReachableValue = System.getProperty("zookeeper.checkIPTimeout");
+ if(checkIPReachableValue != null){
+ this.checkIPReachableTO = Integer.parseInt(checkIPReachableValue);
+ }
}
// VisibleForTesting
public QuorumServer(long id, InetSocketAddress addr) {
this.id = id;
this.addr = addr;
this.electionAddr = null;
+ String checkIPReachableValue = System.getProperty("zookeeper.checkIPTimeout");
+ if(checkIPReachableValue != 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 issue #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/173
@JiangJiafu What I meant is we can solve the problem (sharing code between this issue and ZOOKEEPER-2181) later, after both issues are merged in. We can make things progress by doing one step at a time.
Regarding your question of 3.4.10, are you intended to have this patch being part of the incoming 3.4.10 release?
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r116799126
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -117,13 +117,21 @@ private QuorumServer(long id, InetSocketAddress addr,
this.id = id;
this.addr = addr;
this.electionAddr = electionAddr;
+ String ipReachableValue = System.getProperty("zookeeper.ipReachableTimeout");
--- End diff --
This code is duplicated four times, not good :) - you can pull this into a dedicated function instead. Also make sure to add try catch around the parseInt to prevent crash against illegal input values.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/173
sure, one is on the client and one is on the server but I do not think that means the problem is inherently different.
I think it would be fair to say that both pr's revolve around what to do when the answer we select from dns is not one that we could use (they both revolve around calls to `InetAddress.getAllByName`). This patch description is framed in a way where we are assuming there is a list of possible ip addresses for some hostname and we just need to find one that is working. Nevertheless, this patch should work when the address the hostname resolves to changes entirely.
Likewise, ZOOKEEPER-2184 should handle the "first record doesn't work" issue you are solving here.
I think it may be valuable to have some shared code that effectively deals with mapping ip addresses to hostnames.
After an offline discussion with @hanm we concluded that this may be good to look into after both 2691 and 2184 are merged in.
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/173
@JiangJiafu Apologize for lagging on code review. I think this patch still needs a little bit work to get it merged:
* Provide a way to use old address creation function by checking the sys property (See my comment in code.).
* Documentation (see Abe's comment)
* Typo (see Edward's comment)
---
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 #173: ZOOKEEPER-2691: recreateSocketAddresses may rec...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/173#discussion_r101843864
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -113,13 +113,21 @@ private QuorumServer(long id, InetSocketAddress addr,
this.id = id;
this.addr = addr;
this.electionAddr = electionAddr;
+ String checkIPReachableValue = System.getProperty("zookeeper.checkIPTimeout");
+ if(checkIPReachableValue != 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 issue #173: ZOOKEEPER-2691: recreateSocketAddresses may recreate t...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/173
on the surface this pr reminds me of ZOOKEEPER-2184, wondering if some of the logic can be shared? we seem to be solving the same problem of dns resolutions not giving the "best" answers in more than one place.
---
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.
---