zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From anmolnar <...@git.apache.org>
Subject [GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Date Fri, 22 Jun 2018 10:15:08 GMT
Github user anmolnar commented on a diff in the pull request:

    --- Diff: src/java/test/org/apache/zookeeper/test/HierarchicalQuorumTest.java ---
    @@ -226,38 +227,108 @@ void startServers(boolean withObservers) throws Exception {
                 LOG.info(hp + " is accepting client connections");
    +        LOG.info("initial status " + s1.getCurrentVote() + "," + s2.getCurrentVote()
+ "," + s3.getCurrentVote());
    --- End diff --
    I think this is not right approach for testing this. The patch is about adding a new field
to a JMX bean to expose a new attribute of ZooKeeper. This test is part of `ClientHammerTest`:
completely different things. For example, if the new functionality got broken and JMX value
is not exposed correctly, we'll see that `ClientHammerTest` is failing which is terribly misleading.
    Please revert changes to this file and add more unit tests to `RemotePeerBeenTest`, because
that is essentially what you've changed in this patch and you want to validate. I'd do something
like this in a new test:
    QuorumPeer peerMock = mock(QuorumPeer.class);
    RemotePeerBean remotePeerBean = new RemotePeerBean(peerMock, new QuorumServer(5, ...));
    assertThat("Remote peer bean should expose isLeader() property of remote peer", remotePeerBean.isLeader(5),
    Haven't tested the code, sorry if it's not working out of the box, but hopefully give
you an idea.


View raw message