[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-07-27 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/zookeeper/pull/548


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-07-19 Thread mjeelanimsft 
Github user mjeelanimsft commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r203817750
  
--- Diff: src/java/main/org/apache/zookeeper/server/util/ConfigUtils.java 
---
@@ -61,10 +61,16 @@ static public String getClientConfigStr(String 
configData) {
 return version + " " + sb.toString();
 }
 
+/**
+ * Splits server config to server and port
+ * with support for IPv6 literals
+ * @return String[] first element being the
+ *  IP address and the next being the port
+ * @param s server config, server:port
+ */
 public static String[] splitServerConfig(String s)
 throws ConfigException
--- End diff --

Sure, I like that better too - I've renamed this


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-07-19 Thread mjeelanimsft 
Github user mjeelanimsft commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r203816489
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -857,15 +869,15 @@ public void run() {
 self.recreateSocketAddresses(self.getId());
 addr = self.getElectionAddress();
 }
-LOG.info("My election bind port: " + addr.toString());
+LOG.info("My election bind port: " + 
formatInetAddr(addr));
 setName(addr.toString());
--- End diff --

Good point - I'll remove formatInetAddr(addr) from the log


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-07-12 Thread maoling 
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r201923508
  
--- Diff: src/java/main/org/apache/zookeeper/server/util/ConfigUtils.java 
---
@@ -61,10 +61,16 @@ static public String getClientConfigStr(String 
configData) {
 return version + " " + sb.toString();
 }
 
+/**
+ * Splits server config to server and port
+ * with support for IPv6 literals
+ * @return String[] first element being the
+ *  IP address and the next being the port
+ * @param s server config, server:port
+ */
 public static String[] splitServerConfig(String s)
 throws ConfigException
--- End diff --

`splitServerConfig` is somewhat general. `getHostAndPort` is better?


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-07-12 Thread maoling 
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r201901490
  
--- Diff: 
src/java/test/org/apache/zookeeper/server/util/ConfigUtilsTest.java ---
@@ -29,4 +30,21 @@ public void testSplitServerConfig() throws 
ConfigException {
 String[] nsa = 
ConfigUtils.splitServerConfig("[2001:db8:85a3:8d3:1319:8a2e:370:7348]:443");
 assertEquals(nsa.length, 2, 0);
--- End diff --

Assert the actual host and port?


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-07-12 Thread maoling 
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r201923793
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -226,17 +230,25 @@ static public InitialMessage parse(Long 
protocolVersion, DataInputStream din)
 // FIXME: IPv6 is not supported. Using something like Guava's 
HostAndPort
 //parser would be good.
--- End diff --

remove this ?


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-07-12 Thread maoling 
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r201926608
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -857,15 +869,15 @@ public void run() {
 self.recreateSocketAddresses(self.getId());
 addr = self.getElectionAddress();
 }
-LOG.info("My election bind port: " + addr.toString());
+LOG.info("My election bind port: " + 
formatInetAddr(addr));
 setName(addr.toString());
--- End diff --

using  `toString` can still distinguish ipv4 from ipv6 by looking at the ip 
style although without `[] ` 
if `formatInetAddr(addr)` is really needed, can you make sure all the 
places which log `InetSocketAddress` are covered?


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-07-04 Thread maoling 
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r200064561
  
--- Diff: src/java/main/org/apache/zookeeper/server/util/ConfigUtils.java 
---
@@ -60,4 +60,25 @@ static public String getClientConfigStr(String 
configData) {
 }
 return version + " " + sb.toString();
 }
+
+public static String[] splitServerConfig(String s)
+throws ConfigException
+{
+/* Does it start with an IPv6 literal? */
--- End diff --

remove this line annotation and give this method detailed javadoc


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-26 Thread lvfangmin 
Github user lvfangmin commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r198228442
  
--- Diff: src/java/main/org/apache/zookeeper/client/FourLetterWordMain.java 
---
@@ -86,7 +86,7 @@ public static String send4LetterWord(String host, int 
port, String cmd, boolean
 throws IOException, SSLContextException {
 LOG.info("connecting to {} {}", host, port);
 Socket sock;
-InetSocketAddress hostaddress= host != null ? new 
InetSocketAddress(host, port) :
+InetSocketAddress hostaddress = host != null ? new 
InetSocketAddress(host, port) :
--- End diff --

@mjeelanimsft generally, this is due to the auto-format in the editor, but 
I think we should remove this change from this diff, given we haven't changed 
other logic except this one.


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-26 Thread lvfangmin 
Github user lvfangmin commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r198228805
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java ---
@@ -225,17 +229,25 @@ static public InitialMessage parse(Long 
protocolVersion, DataInputStream din)
 // FIXME: IPv6 is not supported. Using something like Guava's 
HostAndPort
 //parser would be good.
 String addr = new String(b);
-String[] host_port = addr.split(":");
+String[] host_port;
+try {
+host_port = ConfigUtils.splitServerConfig(addr);
+} catch (ConfigException e) {
+throw new InitialMessageException("Badly formed address: 
%s", addr);
+}
 
 if (host_port.length != 2) {
 throw new InitialMessageException("Badly formed address: 
%s", addr);
 }
+//String[] host_port = addr.split(":");
--- End diff --

Remove this line.


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-26 Thread lvfangmin 
Github user lvfangmin commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r198229351
  
--- Diff: 
src/java/test/org/apache/zookeeper/server/quorum/ReconfigFailureCasesTest.java 
---
@@ -58,10 +58,10 @@ public void tearDown() throws Exception {
 }
 
 /*
- * Tests that an incremental reconfig fails if the current config is 
hiearchical.
+ * Tests that an incremental reconfig fails if the current config is 
hierarchical.
--- End diff --

Please remove this unrelated change as well.


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-26 Thread lvfangmin 
Github user lvfangmin commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r198227173
  
--- Diff: src/java/main/org/apache/zookeeper/client/FourLetterWordMain.java 
---
@@ -86,7 +86,7 @@ public static String send4LetterWord(String host, int 
port, String cmd, boolean
 throws IOException, SSLContextException {
 LOG.info("connecting to {} {}", host, port);
 Socket sock;
-InetSocketAddress hostaddress= host != null ? new 
InetSocketAddress(host, port) :
+InetSocketAddress hostaddress = host != null ? new 
InetSocketAddress(host, port) :
--- End diff --

This is generally due to some auto format done in editor.


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-25 Thread mjeelanimsft 
Github user mjeelanimsft commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r197976612
  
--- Diff: src/java/test/org/apache/zookeeper/common/NetUtilsTest.java ---
@@ -0,0 +1,46 @@
+package apache.zookeeper.common;
+
+import org.apache.zookeeper.common.NetUtils;
+import org.apache.zookeeper.ZKTestCase;
+import org.hamcrest.core.AnyOf;
+import org.hamcrest.core.IsEqual;
+import org.junit.Assert;
+import org.junit.Test;
+import java.net.InetSocketAddress;
+
+public class NetUtilsTest extends ZKTestCase {
+
+private Integer port = 1234;
+private String v4addr = "127.0.0.1";
+private String v6addr = "[0:0:0:0:0:0:0:1]";
+private String v4local = v4addr + ":" + port.toString();
+private String v6local = v6addr + ":" + port.toString();
+
+@Test
+public void testformatInetAddrGoodIpv4() {
+InetSocketAddress isa = new InetSocketAddress(v4addr, port);
+Assert.assertEquals("127.0.0.1:1234", 
NetUtils.formatInetAddr(isa));
+}
+
+@Test
+public void testFormatInetAddrGoodIpv6() {
--- End diff --

Thanks @anmolnar - I'll add the additional tests


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-25 Thread mjeelanimsft 
Github user mjeelanimsft commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r197950541
  
--- Diff: src/java/test/org/apache/zookeeper/common/NetUtilsTest.java ---
@@ -0,0 +1,46 @@
+package apache.zookeeper.common;
--- End diff --

Thanks @maoling @breed - working on fixing this


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-25 Thread breed 
Github user breed commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r197933819
  
--- Diff: src/java/test/org/apache/zookeeper/common/NetUtilsTest.java ---
@@ -0,0 +1,46 @@
+package apache.zookeeper.common;
--- End diff --

@mjeelanimsft i think this is why the tests are failing. can you fix this 
package name? (good catch @maoling !)


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-25 Thread anmolnar 
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r197781125
  
--- Diff: src/java/test/org/apache/zookeeper/common/NetUtilsTest.java ---
@@ -0,0 +1,46 @@
+package apache.zookeeper.common;
+
+import org.apache.zookeeper.common.NetUtils;
+import org.apache.zookeeper.ZKTestCase;
+import org.hamcrest.core.AnyOf;
+import org.hamcrest.core.IsEqual;
+import org.junit.Assert;
+import org.junit.Test;
+import java.net.InetSocketAddress;
+
+public class NetUtilsTest extends ZKTestCase {
+
+private Integer port = 1234;
+private String v4addr = "127.0.0.1";
+private String v6addr = "[0:0:0:0:0:0:0:1]";
+private String v4local = v4addr + ":" + port.toString();
+private String v6local = v6addr + ":" + port.toString();
+
+@Test
+public void testformatInetAddrGoodIpv4() {
+InetSocketAddress isa = new InetSocketAddress(v4addr, port);
+Assert.assertEquals("127.0.0.1:1234", 
NetUtils.formatInetAddr(isa));
+}
+
+@Test
+public void testFormatInetAddrGoodIpv6() {
--- End diff --

I think it'd be great to add more IPv6 formatting tests other than the 
localhost case.


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-25 Thread anmolnar 
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r197780621
  
--- Diff: src/java/test/org/apache/zookeeper/common/NetUtilsTest.java ---
@@ -0,0 +1,46 @@
+package apache.zookeeper.common;
+
+import org.apache.zookeeper.common.NetUtils;
+import org.apache.zookeeper.ZKTestCase;
+import org.hamcrest.core.AnyOf;
+import org.hamcrest.core.IsEqual;
+import org.junit.Assert;
+import org.junit.Test;
+import java.net.InetSocketAddress;
+
+public class NetUtilsTest extends ZKTestCase {
+
+private Integer port = 1234;
+private String v4addr = "127.0.0.1";
+private String v6addr = "[0:0:0:0:0:0:0:1]";
+private String v4local = v4addr + ":" + port.toString();
+private String v6local = v6addr + ":" + port.toString();
+
+@Test
+public void testformatInetAddrGoodIpv4() {
--- End diff --

Typo: capital `F`


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-25 Thread anmolnar 
Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r197779656
  
--- Diff: src/java/main/org/apache/zookeeper/client/FourLetterWordMain.java 
---
@@ -86,7 +86,7 @@ public static String send4LetterWord(String host, int 
port, String cmd, boolean
 throws IOException, SSLContextException {
 LOG.info("connecting to {} {}", host, port);
 Socket sock;
-InetSocketAddress hostaddress= host != null ? new 
InetSocketAddress(host, port) :
+InetSocketAddress hostaddress = host != null ? new 
InetSocketAddress(host, port) :
--- End diff --

Agreed, it's not ideal, but size of the patch makes it easy to deal with I 
would say.


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-24 Thread maoling 
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r197633793
  
--- Diff: src/java/main/org/apache/zookeeper/client/FourLetterWordMain.java 
---
@@ -86,7 +86,7 @@ public static String send4LetterWord(String host, int 
port, String cmd, boolean
 throws IOException, SSLContextException {
 LOG.info("connecting to {} {}", host, port);
 Socket sock;
-InetSocketAddress hostaddress= host != null ? new 
InetSocketAddress(host, port) :
+InetSocketAddress hostaddress = host != null ? new 
InetSocketAddress(host, port) :
--- End diff --

it seems that this PR includes some code formats and typos which isn't 
related.


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-24 Thread maoling 
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r197633896
  
--- Diff: src/java/test/org/apache/zookeeper/common/NetUtilsTest.java ---
@@ -0,0 +1,46 @@
+package apache.zookeeper.common;
--- End diff --

???


---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-24 Thread maoling 
Github user maoling commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/548#discussion_r197633888
  
--- Diff: src/java/main/org/apache/zookeeper/server/util/ConfigUtils.java 
---
@@ -60,4 +60,25 @@ static public String getClientConfigStr(String 
configData) {
 }
 return version + " " + sb.toString();
 }
+
+public static String[] splitServerConfig(String s)
+throws ConfigException
+{
+/* Does it start with an IPv6 literal? */
--- End diff --

good method.this annotation's answer is sure(grin)






---

[GitHub] zookeeper pull request #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

2018-06-22 Thread mjeelanimsft 
GitHub user mjeelanimsft opened a pull request:

https://github.com/apache/zookeeper/pull/548

[ZOOKEEPER-3057] Fix IPv6 literal usage

This patch contains fixes for IPv6 literal usage and corresponding unit 
test changes. 

As per discussion in ZOOKEEPER-3057 - The issue/problem is the same as 
ZOOKEEPER-2989, but we changed the code to pass IPv6 literal [%s]:%s, also we 
changed the logging and the LocalPeerBean to show this IPv6 literal as well, 
which makes it easier to check when using Ipv6 and we added detailed tests for 
this change, sending out for review to see if it's better or not.

ZKPatch: 88e94e6f3665353446bf70a042c8f0cd50834f7c (extract)

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/mjeelanimsft/zookeeper fix-ipv6-literal

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/548.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 #548


commit 196d6c3758df8a86cce405ce9552ea72d2cca18a
Author: Jeelani Mohamed Abdul Khader 
Date:   2018-06-06T01:58:26Z

Fix IPv6 literal usage

ZKPatch: 88e94e6f3665353446bf70a042c8f0cd50834f7c (extract)




---