From dev-return-70767-archive-asf-public=cust-asf.ponee.io@zookeeper.apache.org Fri Jun 22 12:15:10 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 47FF818067A for ; Fri, 22 Jun 2018 12:15:10 +0200 (CEST) Received: (qmail 62067 invoked by uid 500); 22 Jun 2018 10:15:09 -0000 Mailing-List: contact dev-help@zookeeper.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zookeeper.apache.org Delivered-To: mailing list dev@zookeeper.apache.org Received: (qmail 62009 invoked by uid 99); 22 Jun 2018 10:15:08 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 22 Jun 2018 10:15:08 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 8B9ABDFAB3; Fri, 22 Jun 2018 10:15:08 +0000 (UTC) From: anmolnar To: dev@zookeeper.apache.org Reply-To: dev@zookeeper.apache.org References: In-Reply-To: Subject: [GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i... Content-Type: text/plain Message-Id: <20180622101508.8B9ABDFAB3@git1-us-west.apache.org> Date: Fri, 22 Jun 2018 10:15:08 +0000 (UTC) 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. ---