[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/648 Thanks ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/648 Committed. Thanks @revans2 ! Please close this PR. ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/648 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2551/ ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/648 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2469/ ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/648 retest this please ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/648 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2467/ ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/648 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2347/ ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/648 Thanks for all of the reviews I just rebased and squashed commits. ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/648 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2322/ ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/648 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2323/ ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/648 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2326/ ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/648 There were some comments in the other pull request about the test code. I will address them in both places. ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/648 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2302/ ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/648 I just added in a test and made the code match closer to how master/3.5 work for the canonical host name. ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/648 I tried to add in powermock too, but it conflicts with mockito (power mock uses a much older version of mockito-core). I'll try and use a different API for powermock instead of mockito. ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/648 Looks like it is mockito-all not powermock-api-mockito. I can try and change it and see. ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/648 Do we have PowerMockito on ZK classpath for tests ? ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/648 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2299/ ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/648 So I am running into some issues with the tests as all of the methods to InetSocketAddress are marked as final and as such I cannot mock them. This means I would need a set of host names that are real, and accessible from any build host you would want to run the test on and never going to be taken down, or I would need to create another class that wraps InetSocketAddress for the canonicalization that would allow me to do the mocking at that level. Not sure which you would prefer. ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/648 Sorry yes. I'll get on the test case... ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/648 Overall looks good to me What about adding a test case ? ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/648 I just addressed the review comments if things look good I am happy to squash commits. I'll also put up pull requests to the other lines, now that it looks like we have the majority of the code worked out. ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/648 @eolivelli I like your idea about the unit test. ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/648 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2284/ ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/648 @anmolnar I think I have addressed all of your comments. Please have another look. ---
[GitHub] zookeeper issue #648: ZOOKEEPER-3156: Add in option to canonicalize host nam...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/648 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2282/ ---
