You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by mi...@cs.stanford.edu on 2014/03/21 00:52:44 UTC

Review Request 19508: ZOOKEEPER-1894: ObserverTest.testObserver fails consistently

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19508/
-----------------------------------------------------------

Review request for zookeeper.


Repository: zookeeper


Description
-------

Add a method to get the hostname or the ip address of InetSocketAddress without performing a reverse DNS lookup. It's basically a substitute for java.net.InetSocketAddress#getHostString(), which is only available since java 7.


Diffs
-----

  http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/common/HostNameUtils.java PRE-CREATION 
  http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1579834 
  http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/test/org/apache/zookeeper/common/HostNameUtilsTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/19508/diff/


Testing
-------

Added unit tests, and verified the patch fixes ObserverTest.testObserver.


Thanks,

michim

Re: Review Request 19508: ZOOKEEPER-1894: ObserverTest.testObserver fails consistently

Posted by Camille Fournier <sk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19508/#review38684
-----------------------------------------------------------


Other than that issue, this lgtm. Fix it and I will check it in.


http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/test/org/apache/zookeeper/common/HostNameUtilsTest.java
<https://reviews.apache.org/r/19508/#comment70982>

    This test does not test anything in ZooKeeper, so we should not have it.


- Camille Fournier


On March 26, 2014, 11:05 p.m., michim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19508/
> -----------------------------------------------------------
> 
> (Updated March 26, 2014, 11:05 p.m.)
> 
> 
> Review request for zookeeper.
> 
> 
> Repository: zookeeper
> 
> 
> Description
> -------
> 
> Add a method to get the hostname or the ip address of InetSocketAddress without performing a reverse DNS lookup. It's basically a substitute for java.net.InetSocketAddress#getHostString(), which is only available since java 7.
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/common/HostNameUtils.java PRE-CREATION 
>   http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1582109 
>   http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/test/org/apache/zookeeper/common/HostNameUtilsTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19508/diff/
> 
> 
> Testing
> -------
> 
> Added unit tests, and verified the patch fixes ObserverTest.testObserver.
> 
> 
> Thanks,
> 
> michim
> 
>

Re: Review Request 19508: ZOOKEEPER-1894: ObserverTest.testObserver fails consistently

Posted by mi...@cs.stanford.edu.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19508/
-----------------------------------------------------------

(Updated March 26, 2014, 11:05 p.m.)


Review request for zookeeper.


Changes
-------

Addressed Rakesh's comments.


Repository: zookeeper


Description
-------

Add a method to get the hostname or the ip address of InetSocketAddress without performing a reverse DNS lookup. It's basically a substitute for java.net.InetSocketAddress#getHostString(), which is only available since java 7.


Diffs
-----

  http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/common/HostNameUtils.java PRE-CREATION 
  http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1582109 
  http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/test/org/apache/zookeeper/common/HostNameUtilsTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/19508/diff/


Testing
-------

Added unit tests, and verified the patch fixes ObserverTest.testObserver.


Thanks,

michim

Re: Review Request 19508: ZOOKEEPER-1894: ObserverTest.testObserver fails consistently

Posted by mi...@cs.stanford.edu.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19508/
-----------------------------------------------------------

(Updated March 26, 2014, 11:04 p.m.)


Review request for zookeeper.


Changes
-------

Addressed Rakesh's comments.


Repository: zookeeper


Description
-------

Add a method to get the hostname or the ip address of InetSocketAddress without performing a reverse DNS lookup. It's basically a substitute for java.net.InetSocketAddress#getHostString(), which is only available since java 7.


Diffs (updated)
-----

  http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/common/HostNameUtils.java PRE-CREATION 
  http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1582109 
  http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/test/org/apache/zookeeper/common/HostNameUtilsTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/19508/diff/


Testing
-------

Added unit tests, and verified the patch fixes ObserverTest.testObserver.


Thanks,

michim

Re: Review Request 19508: ZOOKEEPER-1894: ObserverTest.testObserver fails consistently

Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19508/#review38412
-----------------------------------------------------------


Thanks Michi for the patch, it looks very nice. I've just few minor suggestions, kindly see it.


http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
<https://reviews.apache.org/r/19508/#comment70565>

    Since we are touching this, can we append the strings instead of concatenation ?
    
    like: 
    
    sw.append(HostNameUtils.getHostString(clientAddr));
    sw.append(":"); 
    sw.append(String.valueOf(clientAddr.getPort())); 
    



http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/test/org/apache/zookeeper/common/HostNameUtilsTest.java
<https://reviews.apache.org/r/19508/#comment70561>

    Very good test cases. 
    
    It would be nice if we add the message section in the Assert.assertEquals("message", expAg, actualArg) like:
    
    Assert.assertEquals("Failed to resolve InetSocketAddress with no host to 0.0.0.0", socketAddress.getAddress().getHostAddress(),  HostNameUtils.getHostString(socketAddress);
    
    Also it would be great if we can modify other assertions present in the tests too.
    
    


- Rakesh R


On March 20, 2014, 11:52 p.m., michim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19508/
> -----------------------------------------------------------
> 
> (Updated March 20, 2014, 11:52 p.m.)
> 
> 
> Review request for zookeeper.
> 
> 
> Repository: zookeeper
> 
> 
> Description
> -------
> 
> Add a method to get the hostname or the ip address of InetSocketAddress without performing a reverse DNS lookup. It's basically a substitute for java.net.InetSocketAddress#getHostString(), which is only available since java 7.
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/common/HostNameUtils.java PRE-CREATION 
>   http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1579834 
>   http://svn.apache.org/repos/asf/zookeeper/trunk/src/java/test/org/apache/zookeeper/common/HostNameUtilsTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19508/diff/
> 
> 
> Testing
> -------
> 
> Added unit tests, and verified the patch fixes ObserverTest.testObserver.
> 
> 
> Thanks,
> 
> michim
> 
>