You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by eolivelli <gi...@git.apache.org> on 2018/06/20 13:21:27 UTC
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
GitHub user eolivelli opened a pull request:
https://github.com/apache/zookeeper/pull/546
ZOOKEEPER-3066 Expose on JMX of Followers the id of the current leader
Expose a new JMX attribute "isLeader" in RemotePeerBean and LocalPeerBean
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/eolivelli/zookeeper fix/ZOOKEEPER-3066-leader-jmx
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/546.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #546
----
commit beee9324db4d0de9e874fec0aa0e9cc19dde9b79
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-20T13:20:18Z
ZOOKEEPER-3066 Expose on JMX of Followers the id of the current leader
Export a new JMX attribute isLeader in RemotePeerBean and LocalPeerBean
----
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197768825
--- Diff: src/java/test/org/apache/zookeeper/server/quorum/RemotePeerBeanTest.java ---
@@ -36,7 +36,7 @@ public void testGetClientAddressShouldReturnEmptyStringWhenClientAddressIsNull()
InetSocketAddress peerCommunicationAddress = null;
// Here peerCommunicationAddress is null, also clientAddr is null
QuorumServer peer = new QuorumServer(1, peerCommunicationAddress);
- RemotePeerBean remotePeerBean = new RemotePeerBean(peer);
+ RemotePeerBean remotePeerBean = new RemotePeerBean(null /*QuorumPeer*/, peer);
--- End diff --
@ivankelly @anmolnar I will be happy to follow the guidelines on ZK codebase, just tell me
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197496922
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -2069,4 +2073,9 @@ public QuorumCnxManager createCnxnManager() {
this.quorumCnxnThreadsSize,
this.isQuorumSaslAuthEnabled());
}
+
+ boolean isLeader(long id) {
+ Vote vote = getCurrentVote();
+ return vote != null && id == vote.getId();
--- End diff --
@anmolnar
Do I have to check in the current view?
Is this about temporary leader election phase? I don't know the code very well
can you please give an hint about this new test?
Do I have to simply test the check I am adding? Like a basic unit test with mockito?
---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Posted by anmolnar <gi...@git.apache.org>.
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...
Posted by eolivelli <gi...@git.apache.org>.
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...
Posted by eolivelli <gi...@git.apache.org>.
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...
Posted by eolivelli <gi...@git.apache.org>.
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...
Posted by eolivelli <gi...@git.apache.org>.
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 test....I 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...
Posted by eolivelli <gi...@git.apache.org>.
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by anmolnar <gi...@git.apache.org>.
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.
---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Posted by anmolnar <gi...@git.apache.org>.
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...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/546
@anmolnar ready to go now
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli closed the pull request at:
https://github.com/apache/zookeeper/pull/546
---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Posted by anmolnar <gi...@git.apache.org>.
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197485176
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -2069,4 +2073,9 @@ public QuorumCnxManager createCnxnManager() {
this.quorumCnxnThreadsSize,
this.isQuorumSaslAuthEnabled());
}
+
+ boolean isLeader(long id) {
+ Vote vote = getCurrentVote();
+ return vote != null && id == vote.getId();
--- End diff --
I think it would be good to add unit test to verify this logic too.
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197395687
--- Diff: src/java/test/org/apache/zookeeper/server/quorum/RemotePeerBeanTest.java ---
@@ -36,7 +36,7 @@ public void testGetClientAddressShouldReturnEmptyStringWhenClientAddressIsNull()
InetSocketAddress peerCommunicationAddress = null;
// Here peerCommunicationAddress is null, also clientAddr is null
QuorumServer peer = new QuorumServer(1, peerCommunicationAddress);
- RemotePeerBean remotePeerBean = new RemotePeerBean(peer);
+ RemotePeerBean remotePeerBean = new RemotePeerBean(null /*QuorumPeer*/, peer);
--- End diff --
Do you think that comment could be useful? Modern IDEs like intellij give hints about null arguments and the comment seems to be redundant to me.
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by ivankelly <gi...@git.apache.org>.
Github user ivankelly commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r196823442
--- Diff: src/java/test/org/apache/zookeeper/test/HierarchicalQuorumTest.java ---
@@ -227,37 +227,73 @@ void startServers(boolean withObservers) throws Exception {
LOG.info(hp + " is accepting client connections");
}
+ final int numberOfPeers = 5;
+
// interesting to see what's there...
JMXEnv.dump();
// make sure we have these 5 servers listed
Set<String> ensureNames = new LinkedHashSet<String>();
- for (int i = 1; i <= 5; i++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
ensureNames.add("InMemoryDataTree");
}
- for (int i = 1; i <= 5; i++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
ensureNames.add("name0=ReplicatedServer_id" + i
+ ",name1=replica." + i + ",name2=");
}
- for (int i = 1; i <= 5; i++) {
- for (int j = 1; j <= 5; j++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
+ for (int j = 1; j <= numberOfPeers; j++) {
ensureNames.add("name0=ReplicatedServer_id" + i
+ ",name1=replica." + j);
}
}
- for (int i = 1; i <= 5; i++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
ensureNames.add("name0=ReplicatedServer_id" + i);
}
JMXEnv.ensureAll(ensureNames.toArray(new String[ensureNames.size()]));
-
- for (int i = 1; i <= 5; i++) {
+ int countLeadersUsingLocalPeerBean = 0;
+ for (int i = 1; i <= numberOfPeers; i++) {
+ // LocalPeerBean
String bean = CommonNames.DOMAIN + ":name0=ReplicatedServer_id" + i
+ ",name1=replica." + i;
JMXEnv.ensureBeanAttribute(bean, "ConfigVersion");
JMXEnv.ensureBeanAttribute(bean, "LearnerType");
JMXEnv.ensureBeanAttribute(bean, "ClientAddress");
JMXEnv.ensureBeanAttribute(bean, "ElectionAddress");
JMXEnv.ensureBeanAttribute(bean, "QuorumSystemInfo");
+ boolean leader = (boolean) JMXEnv.ensureBeanAttribute(bean, "Leader");
+ if (leader) {
+ countLeadersUsingLocalPeerBean++;
+ }
}
+ Assert.assertEquals(1, countLeadersUsingLocalPeerBean);
+
+
+ int countLeadersUseRemotePeerBean = 0;
--- End diff --
Would be good to test that the jmx is correctly updated if the leader changes.
---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Posted by eolivelli <gi...@git.apache.org>.
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/546
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197833005
--- Diff: src/java/test/org/apache/zookeeper/server/quorum/LocalPeerBeanTest.java ---
@@ -79,4 +80,27 @@ public void testClientAddress() throws Exception {
cnxnFactory.shutdown();
}
+ @Test
+ @SuppressWarnings("unchecked")
+ public void testIsLeader() throws Exception {
--- End diff --
- This is a `QuorumPeer` test, move it to `QuorumPeerTest` class please.
- These are actually 3 tests together, split them into 3 methods please.
---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Posted by eolivelli <gi...@git.apache.org>.
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
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197484891
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -2069,4 +2073,9 @@ public QuorumCnxManager createCnxnManager() {
this.quorumCnxnThreadsSize,
this.isQuorumSaslAuthEnabled());
}
+
+ boolean isLeader(long id) {
+ Vote vote = getCurrentVote();
+ return vote != null && id == vote.getId();
--- End diff --
You could use `state` property to check if the peer is the leader:
```java
return state == ServerState.LEADING;
```
What do you think?
---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/546
retest this please
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197072479
--- Diff: src/java/test/org/apache/zookeeper/test/HierarchicalQuorumTest.java ---
@@ -226,38 +226,71 @@ void startServers(boolean withObservers) throws Exception {
CONNECTION_TIMEOUT));
LOG.info(hp + " is accepting client connections");
}
-
+ final int numberOfPeers = 5;
// interesting to see what's there...
JMXEnv.dump();
// make sure we have these 5 servers listed
Set<String> ensureNames = new LinkedHashSet<String>();
- for (int i = 1; i <= 5; i++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
ensureNames.add("InMemoryDataTree");
}
- for (int i = 1; i <= 5; i++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
ensureNames.add("name0=ReplicatedServer_id" + i
+ ",name1=replica." + i + ",name2=");
}
- for (int i = 1; i <= 5; i++) {
- for (int j = 1; j <= 5; j++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
+ for (int j = 1; j <= numberOfPeers; j++) {
ensureNames.add("name0=ReplicatedServer_id" + i
+ ",name1=replica." + j);
}
}
- for (int i = 1; i <= 5; i++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
ensureNames.add("name0=ReplicatedServer_id" + i);
}
JMXEnv.ensureAll(ensureNames.toArray(new String[ensureNames.size()]));
-
- for (int i = 1; i <= 5; i++) {
+ int countLeadersUsingLocalPeerBean = 0;
+ for (int i = 1; i <= numberOfPeers; i++) {
+ // LocalPeerBean
String bean = CommonNames.DOMAIN + ":name0=ReplicatedServer_id" + i
+ ",name1=replica." + i;
JMXEnv.ensureBeanAttribute(bean, "ConfigVersion");
JMXEnv.ensureBeanAttribute(bean, "LearnerType");
JMXEnv.ensureBeanAttribute(bean, "ClientAddress");
JMXEnv.ensureBeanAttribute(bean, "ElectionAddress");
JMXEnv.ensureBeanAttribute(bean, "QuorumSystemInfo");
+ boolean leader = (boolean) JMXEnv.ensureBeanAttribute(bean, "Leader");
+ if (leader) {
+ countLeadersUsingLocalPeerBean++;
+ }
+ }
+ Assert.assertEquals(1, countLeadersUsingLocalPeerBean);
+
+
+ int countLeadersUseRemotePeerBean = 0;
--- End diff --
This whole test could use a refactor (most unit test could), but if we touch a unit test, we could keep good practices in mind. This could go to a seperate function. (But this is not a showstopper for me, just my two cents. )
---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Posted by eolivelli <gi...@git.apache.org>.
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r196841132
--- Diff: src/java/test/org/apache/zookeeper/test/HierarchicalQuorumTest.java ---
@@ -227,37 +227,73 @@ void startServers(boolean withObservers) throws Exception {
LOG.info(hp + " is accepting client connections");
}
+ final int numberOfPeers = 5;
+
// interesting to see what's there...
JMXEnv.dump();
// make sure we have these 5 servers listed
Set<String> ensureNames = new LinkedHashSet<String>();
- for (int i = 1; i <= 5; i++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
ensureNames.add("InMemoryDataTree");
}
- for (int i = 1; i <= 5; i++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
ensureNames.add("name0=ReplicatedServer_id" + i
+ ",name1=replica." + i + ",name2=");
}
- for (int i = 1; i <= 5; i++) {
- for (int j = 1; j <= 5; j++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
+ for (int j = 1; j <= numberOfPeers; j++) {
ensureNames.add("name0=ReplicatedServer_id" + i
+ ",name1=replica." + j);
}
}
- for (int i = 1; i <= 5; i++) {
+ for (int i = 1; i <= numberOfPeers; i++) {
ensureNames.add("name0=ReplicatedServer_id" + i);
}
JMXEnv.ensureAll(ensureNames.toArray(new String[ensureNames.size()]));
-
- for (int i = 1; i <= 5; i++) {
+ int countLeadersUsingLocalPeerBean = 0;
+ for (int i = 1; i <= numberOfPeers; i++) {
+ // LocalPeerBean
String bean = CommonNames.DOMAIN + ":name0=ReplicatedServer_id" + i
+ ",name1=replica." + i;
JMXEnv.ensureBeanAttribute(bean, "ConfigVersion");
JMXEnv.ensureBeanAttribute(bean, "LearnerType");
JMXEnv.ensureBeanAttribute(bean, "ClientAddress");
JMXEnv.ensureBeanAttribute(bean, "ElectionAddress");
JMXEnv.ensureBeanAttribute(bean, "QuorumSystemInfo");
+ boolean leader = (boolean) JMXEnv.ensureBeanAttribute(bean, "Leader");
+ if (leader) {
+ countLeadersUsingLocalPeerBean++;
+ }
}
+ Assert.assertEquals(1, countLeadersUsingLocalPeerBean);
+
+
+ int countLeadersUseRemotePeerBean = 0;
--- End diff --
That's would be useful.
I need to bounce the leader, this will slow down the suite.
Do you have another way to change the leader in tests?
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by ivankelly <gi...@git.apache.org>.
Github user ivankelly commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197768450
--- Diff: src/java/test/org/apache/zookeeper/server/quorum/RemotePeerBeanTest.java ---
@@ -36,7 +36,7 @@ public void testGetClientAddressShouldReturnEmptyStringWhenClientAddressIsNull()
InetSocketAddress peerCommunicationAddress = null;
// Here peerCommunicationAddress is null, also clientAddr is null
QuorumServer peer = new QuorumServer(1, peerCommunicationAddress);
- RemotePeerBean remotePeerBean = new RemotePeerBean(peer);
+ RemotePeerBean remotePeerBean = new RemotePeerBean(null /*QuorumPeer*/, peer);
--- End diff --
Not everyone uses an IDE, and code review happens on github which doesn't give hints about this stuff.
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197604271
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -2069,4 +2073,9 @@ public QuorumCnxManager createCnxnManager() {
this.quorumCnxnThreadsSize,
this.isQuorumSaslAuthEnabled());
}
+
+ boolean isLeader(long id) {
+ Vote vote = getCurrentVote();
+ return vote != null && id == vote.getId();
--- End diff --
@enixon sorry, to me it is not clear if you are saying that current method is okay or that I should change it according to @anmolnar idea.
For LocalPeerBean it may have sense to also check local election state, but for RemotePeerBean I can't see the meaning.
Alternatively we could return 'false' in case that local peer is performing leader election.
I think we should keep the logic simple
---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Posted by anmolnar <gi...@git.apache.org>.
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by enixon <gi...@git.apache.org>.
Github user enixon commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197574036
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -2069,4 +2073,9 @@ public QuorumCnxManager createCnxnManager() {
this.quorumCnxnThreadsSize,
this.isQuorumSaslAuthEnabled());
}
+
+ boolean isLeader(long id) {
+ Vote vote = getCurrentVote();
+ return vote != null && id == vote.getId();
--- End diff --
I'd second the request to sidestep the election logic for this method if possible as it makes the method harder to reason about.
It would be nice if the method implied the server was currently performing active leader actions and the current way also covers a server preparing for or nominating itself to be leader (or at least it looks like it). For example, with this code on a 5 server ensemble with one server dead, if one server switches its vote midway through an election epoch then you could be displaying two "leaders" by vote but this is not the same as a two leader splitbrain scenario.
---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Posted by eolivelli <gi...@git.apache.org>.
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by ivankelly <gi...@git.apache.org>.
Github user ivankelly commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r196821843
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/LocalPeerBean.java ---
@@ -25,7 +25,7 @@
*/
public class LocalPeerBean extends ServerBean implements LocalPeerMXBean {
private final QuorumPeer peer;
-
+
--- End diff --
unrelated, creates noise in patch
---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Posted by eolivelli <gi...@git.apache.org>.
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...
Posted by eolivelli <gi...@git.apache.org>.
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...
Posted by eolivelli <gi...@git.apache.org>.
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...
Posted by eolivelli <gi...@git.apache.org>.
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...
Posted by eolivelli <gi...@git.apache.org>.
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...
Posted by anmolnar <gi...@git.apache.org>.
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by eolivelli <gi...@git.apache.org>.
GitHub user eolivelli reopened a pull request:
https://github.com/apache/zookeeper/pull/546
ZOOKEEPER-3066 Expose on JMX of Followers the id of the current leader
Expose a new JMX attribute "isLeader" in RemotePeerBean and LocalPeerBean
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/eolivelli/zookeeper fix/ZOOKEEPER-3066-leader-jmx
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/546.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #546
----
commit 067aed9efe135cae686eb6b9681f46870eec28db
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-21T01:11:58Z
ZOOKEEPER-3066 Expose on JMX of Followers the id of the current leader
Enhance RemotePeerMXBean and LocalPeerMXBean by adding information about leadership
commit 5f120631f926e85c83d93c14138748e2b803615c
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-21T11:57:42Z
ZOOKEEPER-3066 add servers restart to test case
commit 2778e742799dccbe841c21b018847dc38f4eaf1e
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-22T11:06:21Z
revert "ZOOKEEPER-3066 add servers restart to test case"
This reverts commit 5f120631f926e85c83d93c14138748e2b803615c.
commit 43782fe5949e522931aa2d812ca3a6bbc3438e2d
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-22T11:34:05Z
introduce mockito tests
commit b5000efc316eb4855ca8a85fc5133d9a9f61d963
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-22T15:26:06Z
clean up test
commit 86b116b090574d5906c575c70e9fbc38c5b83f11
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-25T14:20:04Z
enhance tests
commit eb8d1bead655ae63a3e3779d6eb10b4a86fc2c64
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-25T17:56:40Z
split tests
commit a1984ef1bd51f26ceeb44380571d3a691ea8f483
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-25T17:58:01Z
fix typo
commit 81d7bb1ce6fac4d61a2c0e6d9cfe400d7ec61f23
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-26T06:50:05Z
split tests into LocalPeerBeanTest and QuorumPeerTest
commit ded4ce204b549a2e800ee10d5a8ded4a1a0c3649
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-26T06:51:23Z
reorder imports
commit bf09e37d90445b672fdf3e7aeea72ea05c514416
Author: Enrico Olivelli <eo...@...>
Date: 2018-06-26T06:53:26Z
clean up spaces
----
---
[GitHub] zookeeper issue #546: ZOOKEEPER-3066 Expose on JMX of Followers the id of th...
Posted by enixon <gi...@git.apache.org>.
Github user enixon commented on the issue:
https://github.com/apache/zookeeper/pull/546
+1 from me when anmolnar is satisfied :)
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/546#discussion_r197497322
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -2069,4 +2073,9 @@ public QuorumCnxManager createCnxnManager() {
this.quorumCnxnThreadsSize,
this.isQuorumSaslAuthEnabled());
}
+
+ boolean isLeader(long id) {
+ Vote vote = getCurrentVote();
+ return vote != null && id == vote.getId();
--- End diff --
Should I rename the method to isLeaderForJmx or something similar?
I would like to make it clear that this method is only for external monitoring
---