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:

    https://github.com/apache/zookeeper/pull/546#discussion_r197403323
  
    --- Diff: src/java/test/org/apache/zookeeper/test/HierarchicalQuorumTest.java ---
    @@ -226,38 +227,108 @@ void startServers(boolean withObservers) throws Exception {
                                         CONNECTION_TIMEOUT));
                 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:
    
    ```java
    ...
    QuorumPeer peerMock = mock(QuorumPeer.class);
    RemotePeerBean remotePeerBean = new RemotePeerBean(peerMock, new QuorumServer(5, ...));
    when(peerMock.isLeader(peerId)).return(true);
    assertThat("Remote peer bean should expose isLeader() property of remote peer", remotePeerBean.isLeader(5),
isTrue());
    ...
    ```
    
    Haven't tested the code, sorry if it's not working out of the box, but hopefully give
you an idea.


---

Mime
View raw message