You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by maoling <gi...@git.apache.org> on 2018/07/30 13:31:33 UTC
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
GitHub user maoling opened a pull request:
https://github.com/apache/zookeeper/pull/587
ZOOKEEPER-3106: Zookeeper client supports IPv6 address and document the "IPV6 feature"
The related UTs in the `ConnectStringParserTest` have all passed,I also tested it in the real IPv6 ENV.
More details in [ZOOKEEPER-3106](https://issues.apache.org/jira/browse/ZOOKEEPER-3106)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/maoling/zookeeper ZOOKEEPER-3106
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/587.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 #587
----
commit 6942315d8fc66200a55691df9a59eadf13ff0270
Author: maoling <ma...@...>
Date: 2018-07-29T08:42:10Z
ZOOKEEPER-3106: Zookeeper client supports IPv6 address and document the IPV6 feature
----
---
[GitHub] zookeeper issue #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 address...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/587
Thanks @maoling . Go ahead please!
---
[GitHub] zookeeper issue #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 address...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/587
Committed to master. Thanks @maoling !
This patch doesn't apply to branch-3.5, but I think we should fix the issue on it too.
Would you please open a separate pull request?
---
[GitHub] zookeeper issue #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 address...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:
https://github.com/apache/zookeeper/pull/587
@anmolnar
ZOOKEEPER-3106 is dependent with ZOOKEEPER-3057 which has not been appiled into branch-3.5 and branch-3.4 from the [history](https://github.com/apache/zookeeper/pull/548)
I have ping @mjeelanimsft if he is not free,I will port ZOOKEEPER-3057 into branch-3.5 and branch-3.4 firstly.
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r207730177
--- Diff: src/java/main/org/apache/zookeeper/client/ConnectStringParser.java ---
@@ -68,14 +69,26 @@ public ConnectStringParser(String connectString) {
List<String> hostsList = split(connectString,",");
for (String host : hostsList) {
int port = DEFAULT_PORT;
- int pidx = host.lastIndexOf(':');
- if (pidx >= 0) {
- // otherwise : is at the end of the string, ignore
- if (pidx < host.length() - 1) {
- port = Integer.parseInt(host.substring(pidx + 1));
- }
- host = host.substring(0, pidx);
+ if (!connectString.startsWith("[")) {//IPv4
--- End diff --
@lvfangmin the client's parse logic supports default port, it's not easy to reuse the method getHostAndPort
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r207613222
--- Diff: src/java/main/org/apache/zookeeper/client/ConnectStringParser.java ---
@@ -68,14 +69,26 @@ public ConnectStringParser(String connectString) {
List<String> hostsList = split(connectString,",");
for (String host : hostsList) {
int port = DEFAULT_PORT;
- int pidx = host.lastIndexOf(':');
- if (pidx >= 0) {
- // otherwise : is at the end of the string, ignore
- if (pidx < host.length() - 1) {
- port = Integer.parseInt(host.substring(pidx + 1));
- }
- host = host.substring(0, pidx);
+ if (!connectString.startsWith("[")) {//IPv4
--- End diff --
In https://github.com/apache/zookeeper/pull/548, it added ConfigUtils.getHostAndPort, which can split host port in IPV4 or IPV6 format, maybe we should reuse that code here.
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r207535136
--- Diff: src/java/test/org/apache/zookeeper/test/ConnectStringParserTest.java ---
@@ -39,27 +44,44 @@ public void testMultipleServersChrootPath(){
String servers = "10.10.10.1,10.10.10.2";
assertChrootPath(chrootPath,
new ConnectStringParser(servers+chrootPath));
+
+ servers = "[2001:db8:1::242:ac11:2]:2181,[2001:db8:85a3:8d3:1319:8a2e:370:7348]:5678";
--- End diff --
good suggestion
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r207090394
--- Diff: src/java/main/org/apache/zookeeper/client/ConnectStringParser.java ---
@@ -68,14 +69,26 @@ public ConnectStringParser(String connectString) {
List<String> hostsList = split(connectString,",");
for (String host : hostsList) {
int port = DEFAULT_PORT;
- int pidx = host.lastIndexOf(':');
- if (pidx >= 0) {
- // otherwise : is at the end of the string, ignore
- if (pidx < host.length() - 1) {
- port = Integer.parseInt(host.substring(pidx + 1));
- }
- host = host.substring(0, pidx);
+ if (!connectString.startsWith("[")) {//IPv4
+ int pidx = host.lastIndexOf(':');
+ if (pidx >= 0) {
+ // otherwise : is at the end of the string, ignore
+ if (pidx < host.length() - 1) {
+ port = Integer.parseInt(host.substring(pidx + 1));
+ }
+ host = host.substring(0, pidx);
+ }
+ } else { //IPv6
--- End diff --
@enixon thanks for your review.after collecting enough suggestions,I will polish up this issue.
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r223215411
--- Diff: zookeeper-client/zookeeper-client-java/src/main/java/org/apache/zookeeper/client/ConnectStringParser.java ---
@@ -68,14 +71,16 @@ public ConnectStringParser(String connectString) {
List<String> hostsList = split(connectString,",");
for (String host : hostsList) {
int port = DEFAULT_PORT;
- int pidx = host.lastIndexOf(':');
- if (pidx >= 0) {
- // otherwise : is at the end of the string, ignore
- if (pidx < host.length() - 1) {
- port = Integer.parseInt(host.substring(pidx + 1));
+ try {
+ String[] hostAndPort = ConfigUtils.getHostAndPort(host);
+ host = hostAndPort[0];
+ if (hostAndPort.length == 2) {
+ port = Integer.parseInt(hostAndPort[1]);
}
- host = host.substring(0, pidx);
+ } catch (ConfigException e) {
+ e.printStackTrace();
--- End diff --
Nit: use logger
I wonder if we should fail and not swallow the error
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r223347154
--- Diff: zookeeper-client/zookeeper-client-java/src/main/java/org/apache/zookeeper/client/ConnectStringParser.java ---
@@ -68,14 +71,16 @@ public ConnectStringParser(String connectString) {
List<String> hostsList = split(connectString,",");
for (String host : hostsList) {
int port = DEFAULT_PORT;
- int pidx = host.lastIndexOf(':');
- if (pidx >= 0) {
- // otherwise : is at the end of the string, ignore
- if (pidx < host.length() - 1) {
- port = Integer.parseInt(host.substring(pidx + 1));
+ try {
+ String[] hostAndPort = ConfigUtils.getHostAndPort(host);
+ host = hostAndPort[0];
+ if (hostAndPort.length == 2) {
+ port = Integer.parseInt(hostAndPort[1]);
}
- host = host.substring(0, pidx);
+ } catch (ConfigException e) {
+ e.printStackTrace();
--- End diff --
cc @maoling
---
[GitHub] zookeeper issue #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 address...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/587
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2377/
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by enixon <gi...@git.apache.org>.
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r206987004
--- Diff: src/java/main/org/apache/zookeeper/client/ConnectStringParser.java ---
@@ -68,14 +69,26 @@ public ConnectStringParser(String connectString) {
List<String> hostsList = split(connectString,",");
for (String host : hostsList) {
int port = DEFAULT_PORT;
- int pidx = host.lastIndexOf(':');
- if (pidx >= 0) {
- // otherwise : is at the end of the string, ignore
- if (pidx < host.length() - 1) {
- port = Integer.parseInt(host.substring(pidx + 1));
- }
- host = host.substring(0, pidx);
+ if (!connectString.startsWith("[")) {//IPv4
+ int pidx = host.lastIndexOf(':');
+ if (pidx >= 0) {
+ // otherwise : is at the end of the string, ignore
+ if (pidx < host.length() - 1) {
+ port = Integer.parseInt(host.substring(pidx + 1));
+ }
+ host = host.substring(0, pidx);
+ }
+ } else { //IPv6
--- End diff --
purely selfish request - could you add an example to this comment? something like // IPv6 e.g. [2001:db8:1::242:ac11:2]:1234.
Having that on hand made reasoning about the string parsing logic much easier for me.
---
[GitHub] zookeeper issue #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 address...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:
https://github.com/apache/zookeeper/pull/587
bump @anmolnar
---
[GitHub] zookeeper issue #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 address...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:
https://github.com/apache/zookeeper/pull/587
@anmolnar rebased
the flaky test: `ClientPortBindTest.testBindByAddress` is not related
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by tamaashu <gi...@git.apache.org>.
Github user tamaashu commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r207521296
--- Diff: src/java/test/org/apache/zookeeper/test/ConnectStringParserTest.java ---
@@ -39,27 +44,44 @@ public void testMultipleServersChrootPath(){
String servers = "10.10.10.1,10.10.10.2";
assertChrootPath(chrootPath,
new ConnectStringParser(servers+chrootPath));
+
+ servers = "[2001:db8:1::242:ac11:2]:2181,[2001:db8:85a3:8d3:1319:8a2e:370:7348]:5678";
--- End diff --
Would be useful to define static strings for the two IPv6 address, since you use them so often. Less chance to mistype when changing the test.
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/587
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r207727691
--- Diff: src/java/main/org/apache/zookeeper/client/ConnectStringParser.java ---
@@ -68,14 +69,26 @@ public ConnectStringParser(String connectString) {
List<String> hostsList = split(connectString,",");
for (String host : hostsList) {
int port = DEFAULT_PORT;
- int pidx = host.lastIndexOf(':');
- if (pidx >= 0) {
- // otherwise : is at the end of the string, ignore
- if (pidx < host.length() - 1) {
- port = Integer.parseInt(host.substring(pidx + 1));
- }
- host = host.substring(0, pidx);
+ if (!connectString.startsWith("[")) {//IPv4
--- End diff --
The client parse logic support the default port, so it's not easy to reuse the method ConfigUtils.getHostAndPort
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by enixon <gi...@git.apache.org>.
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r206983579
--- Diff: src/java/main/org/apache/zookeeper/client/ConnectStringParser.java ---
@@ -68,14 +69,26 @@ public ConnectStringParser(String connectString) {
List<String> hostsList = split(connectString,",");
for (String host : hostsList) {
int port = DEFAULT_PORT;
- int pidx = host.lastIndexOf(':');
- if (pidx >= 0) {
- // otherwise : is at the end of the string, ignore
- if (pidx < host.length() - 1) {
- port = Integer.parseInt(host.substring(pidx + 1));
- }
- host = host.substring(0, pidx);
+ if (!connectString.startsWith("[")) {//IPv4
+ int pidx = host.lastIndexOf(':');
+ if (pidx >= 0) {
+ // otherwise : is at the end of the string, ignore
+ if (pidx < host.length() - 1) {
+ port = Integer.parseInt(host.substring(pidx + 1));
+ }
+ host = host.substring(0, pidx);
+ }
+ } else { //IPv6
+ int pidx = host.lastIndexOf(':');
+ int bracketIdx = host.lastIndexOf(']');
+ if (pidx >=0 && bracketIdx >=0 && pidx > bracketIdx) {
+ if (pidx < host.length() - 1) {
+ port = Integer.parseInt(host.substring(pidx + 1));
+ }
+ host = host.substring(0, pidx);
+ }
--- End diff --
nit - you've added tabs with your whitespace
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r223567109
--- Diff: zookeeper-client/zookeeper-client-java/src/main/java/org/apache/zookeeper/client/ConnectStringParser.java ---
@@ -68,14 +71,16 @@ public ConnectStringParser(String connectString) {
List<String> hostsList = split(connectString,",");
for (String host : hostsList) {
int port = DEFAULT_PORT;
- int pidx = host.lastIndexOf(':');
- if (pidx >= 0) {
- // otherwise : is at the end of the string, ignore
- if (pidx < host.length() - 1) {
- port = Integer.parseInt(host.substring(pidx + 1));
+ try {
+ String[] hostAndPort = ConfigUtils.getHostAndPort(host);
+ host = hostAndPort[0];
+ if (hostAndPort.length == 2) {
+ port = Integer.parseInt(hostAndPort[1]);
}
- host = host.substring(0, pidx);
+ } catch (ConfigException e) {
+ e.printStackTrace();
--- End diff --
@eolivelli agreed.Since this PR has been merged.Could you plz open another separate pull request to process it?
---
[GitHub] zookeeper issue #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 address...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/587
@maoling please rebase the patch
---
[GitHub] zookeeper issue #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 address...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/587
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2378/
---
[GitHub] zookeeper pull request #587: ZOOKEEPER-3106: Zookeeper client supports IPv6 ...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/587#discussion_r223346987
--- Diff: zookeeper-client/zookeeper-client-java/src/main/java/org/apache/zookeeper/client/ConnectStringParser.java ---
@@ -68,14 +71,16 @@ public ConnectStringParser(String connectString) {
List<String> hostsList = split(connectString,",");
for (String host : hostsList) {
int port = DEFAULT_PORT;
- int pidx = host.lastIndexOf(':');
- if (pidx >= 0) {
- // otherwise : is at the end of the string, ignore
- if (pidx < host.length() - 1) {
- port = Integer.parseInt(host.substring(pidx + 1));
+ try {
+ String[] hostAndPort = ConfigUtils.getHostAndPort(host);
+ host = hostAndPort[0];
+ if (hostAndPort.length == 2) {
+ port = Integer.parseInt(hostAndPort[1]);
}
- host = host.substring(0, pidx);
+ } catch (ConfigException e) {
+ e.printStackTrace();
--- End diff --
@anmolnar I forgot to push "Request changes".
---