You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by mjeelanimsft <gi...@git.apache.org> on 2018/06/22 23:46:45 UTC

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

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 <mj...@...>
Date:   2018-06-06T01:58:26Z

    Fix IPv6 literal usage
    
    ZKPatch: 88e94e6f3665353446bf70a042c8f0cd50834f7c (extract)

----


---

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

Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    @mjeelanimsft Just to clarify my latest comment: I'd to see a new class `ConfigUtilsTest` in which you explicitly verify the behaviour of `splitServerConfig`. Thanks.


---

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

Posted by maoling <gi...@git.apache.org>.
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

Posted by breed <gi...@git.apache.org>.
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

Posted by lvfangmin <gi...@git.apache.org>.
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

Posted by maoling <gi...@git.apache.org>.
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 issue #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    - It seems that zk server side supports ipv6 style like this: `server.1=[2001:db8:1::242:ac11:2]:2888:3888`,but zk client side supports ipv6 like this:`2001:db8:1::242:ac11:2:2181`.we need unify them?[ConnectStringParser](https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/client/ConnectStringParser.java#L71) can reuse that method?
    - IMO,we should document this feature to let user know.(e.g. [here](https://github.com/apache/zookeeper/blob/master/src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml#L1055))


---

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

Posted by mjeelanimsft <gi...@git.apache.org>.
Github user mjeelanimsft commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    Thanks for the feedback @maoling - I've incorporated the below changes. @maoling @anmolnar - let me know what you think
    
    - Assert actual host and port in unit tests for getHostAndPort
    - Renamed splitServerConfig(String s) to getHostAndPort(String s)
    - Removed Fix Me comment in QuorumCnxManager.java
    - No need to formatInetAddr(addr) in QuorumCnxManager.java log.Info - removed it


---

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

Posted by maoling <gi...@git.apache.org>.
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

Posted by anmolnar <gi...@git.apache.org>.
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 issue #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    ping @mjeelanimsft 
    Would you please create separate pull request for branch-3.5 and branch-3.4?


---

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

Posted by mjeelanimsft <gi...@git.apache.org>.
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

Posted by mjeelanimsft <gi...@git.apache.org>.
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

Posted by lvfangmin <gi...@git.apache.org>.
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

Posted by maoling <gi...@git.apache.org>.
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

Posted by lvfangmin <gi...@git.apache.org>.
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 issue #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

Posted by mjeelanimsft <gi...@git.apache.org>.
Github user mjeelanimsft commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    Thanks @anmolnar & @maoling - I've added the Unit Test as well as the JavaDoc for splitServerConfig() method


---

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

Posted by mjeelanimsft <gi...@git.apache.org>.
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

Posted by maoling <gi...@git.apache.org>.
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

Posted by asfgit <gi...@git.apache.org>.
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

Posted by mjeelanimsft <gi...@git.apache.org>.
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

Posted by anmolnar <gi...@git.apache.org>.
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 issue #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

Posted by mjeelanimsft <gi...@git.apache.org>.
Github user mjeelanimsft commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    Hi @anmolnar - I see it here - would you mind looking now?
    https://github.com/mjeelanimsft/zookeeper/blob/1ce5b8bdcf0925d1ce25d16bf70c1d13de44bf08/src/java/test/org/apache/zookeeper/server/util/ConfigUtilsTest.java


---

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

Posted by breed <gi...@git.apache.org>.
Github user breed commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    +1 i'll kick jenkins


---

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

Posted by maoling <gi...@git.apache.org>.
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

Posted by anmolnar <gi...@git.apache.org>.
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 issue #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

Posted by mjeelanimsft <gi...@git.apache.org>.
Github user mjeelanimsft commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    @anmolnar Resolved the merge conflict. You can see the commit with the unit test changes here - https://github.com/apache/zookeeper/pull/548/commits/ff4704984203921071877bdee4efef47b8c2a706


---

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

Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    @mjeelanimsft  I keep trying to refresh the page, but again I can't see the new tests.
    Also the patch has some conflicts with the latest master, would you please resolve them?


---

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

Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    Committed to master branch only, because it conflicts with 3.5.
    @mjeelanimsft Would you please create separate pull request for branch-3.5 and branch-3.4?


---

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

Posted by maoling <gi...@git.apache.org>.
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 issue #548: [ZOOKEEPER-3057] Fix IPv6 literal usage

Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    @mjeelanimsft Now I can see it, thanks.
    I'd write 3 tests at the minimum to validate all if branches in the method:
    With and w/o the `[` prefix and a third one for the invalid case: no matching closing brace.



---

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

Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    Jenkins is green and we got 2 approvals. Committing.


---

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

Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    @mjeelanimsft Sorry, I still can't see unit tests for `splitServerConfig`.


---

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

Posted by mjeelanimsft <gi...@git.apache.org>.
Github user mjeelanimsft commented on the issue:

    https://github.com/apache/zookeeper/pull/548
  
    Thanks Andor - I've added the unit test cases - For the one w/o [ - its also an invalid case for this method as its needed to specify the port. I've added that as one at the end, let me know what you think. 
    
    1) Successful case (IPv6)
    2) Successful case (IPv4)
    3) Invalid case (IPv6 with missing closing brackets)
    4) Invalid case (IPv6 with port but missing brackets)


---

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

Posted by lvfangmin <gi...@git.apache.org>.
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

Posted by maoling <gi...@git.apache.org>.
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 --
    
    ???


---