[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @anmolnar thank you so much. ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/546 Committed to master and 3.5 branches. Thanks @eolivelli We should not commit new features to `branch-3.5`, because it's already beta, but given that this is monitoring-only, I eventually committed. ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @anmolnar green light ! please cherry pick to 4.5 branch ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 retest this please ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @anmolnar how ? Seems that magic Jenkins words 'retest this please' does not work on ZK ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/546 @eolivelli Perfect, thanks! Now you only need to trigger a green build. ;) ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @anmolnar This round: - Explicit unit test for LocalPeerBean, which is using a mock QuorumPeer - Explicit unit test for QuorumPeer all the code changes are now covered. ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user enixon commented on the issue: https://github.com/apache/zookeeper/pull/546 +1 from me when anmolnar is satisfied :) ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/546 Almost. :) Sorry for still complaining: don't use `LocalPeerBean` in the QuorumPeer tests, it could interfere the result. Test `QuorumPeer.isLeader()` directly. ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @anmolnar ready to go now ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @anmolnar tests added as you requested. Now it looks better. I have understood what you meant, I did not use Mockito, I cannot mock the class I am testing ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @anmolnar sure, thanks. I will add tests. I prefer using Mockito in this case, it makes it clear that we are testing only a part of the class ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/546 @eolivelli Sorry, my fault. I missed that your method is used from RemotePeerBeen too with the remote peer id. Which is fine. Regarding unit testing. - validate currentVote == null case - validate id == getId() and id != getId() cases Not sure if you have to use Moquito, because `QuorumPeer` has default constructor. ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @enixon @anmolnar IMHO I answered to all of your questions ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @anmolnar I checked the code, in my opinion the *id* inside current vote is the id of what the local peer thinks the leader is. See for instance updateServerState, in which state is calculated from the current vote. So my code should be correct and there is no need (or it will be an error) to check the *state* variable. About an additional testI don't know what will be the meaning of the test you are suggesting, maybe I am missing something ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @anmolnar test is clean now. please also cherry pick to 3.5 branch. I am using that version ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @anmolnar I see with your point. Will update the patch soon ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/546 Thank you for the quick turnaround @eolivelli . Mockito unit test is cool, exactly what I meant to. I think it's okay to validate the existence of JMX property in the existing test (similar checks were already there), but would you please remove checks for the *value* of the property? I really don't want the hammer test failing in case of an incorrect JMX value. ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @anmolnar @nkalmar @ivankelly Thank you all for helping me with this patch. I have added a bunch of Mockito based tests. About the existing test..I have only added a check on real JMX about isLeader, the test was already there. I can revert the changes on the existing test if you feel strong about it, but IMHO it ia not a big deal ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @nkalmar @ivankelly thanks for your help I have added a restart of the first 3 servers in the cluster. This is enough to see leadership change, but I did not add specific assertions, because I am seeing that the result of the election is subject to change at each execution. I would not like to add a new "flaky" test. I don't know how it is worth to complicate the test. Anyway, I will be happy to follow your guidelines in order to have a significant test. ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 @ivankelly I have cleaned up the patch, restoring all trailing white spaces. I am trying to find the best way to force a leader election in HierarchicalQuorumTest but I am not finding my way. Do you have any hint ? ---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/546 Patch contains a fix on trailing spaces for each file I have touched (I can revert this if you prefer). I would like to cherry pick to 3.5 branch ---
