[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
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...
Github user eolivelli 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...
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 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 Date: 2018-06-21T11:57:42Z ZOOKEEPER-3066 add servers restart to test case commit 2778e742799dccbe841c21b018847dc38f4eaf1e Author: Enrico Olivelli Date: 2018-06-22T11:06:21Z revert "ZOOKEEPER-3066 add servers restart to test case" This reverts commit 5f120631f926e85c83d93c14138748e2b803615c. commit 43782fe5949e522931aa2d812ca3a6bbc3438e2d Author: Enrico Olivelli Date: 2018-06-22T11:34:05Z introduce mockito tests commit b5000efc316eb4855ca8a85fc5133d9a9f61d963 Author: Enrico Olivelli Date: 2018-06-22T15:26:06Z clean up test commit 86b116b090574d5906c575c70e9fbc38c5b83f11 Author: Enrico Olivelli Date: 2018-06-25T14:20:04Z enhance tests commit eb8d1bead655ae63a3e3779d6eb10b4a86fc2c64 Author: Enrico Olivelli Date: 2018-06-25T17:56:40Z split tests commit a1984ef1bd51f26ceeb44380571d3a691ea8f483 Author: Enrico Olivelli Date: 2018-06-25T17:58:01Z fix typo commit 81d7bb1ce6fac4d61a2c0e6d9cfe400d7ec61f23 Author: Enrico Olivelli Date: 2018-06-26T06:50:05Z split tests into LocalPeerBeanTest and QuorumPeerTest commit ded4ce204b549a2e800ee10d5a8ded4a1a0c3649 Author: Enrico Olivelli Date: 2018-06-26T06:51:23Z reorder imports commit bf09e37d90445b672fdf3e7aeea72ea05c514416 Author: Enrico Olivelli Date: 2018-06-26T06:53:26Z clean up spaces ---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
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...
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...
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
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
---
[GitHub] zookeeper pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
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...
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...
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
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 ensureNames = new LinkedHashSet();
-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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
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 ensureNames = new LinkedHashSet();
-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...
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 pull request #546: ZOOKEEPER-3066 Expose on JMX of Followers the i...
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 ensureNames = new LinkedHashSet();
-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 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 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 ---
