You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by rakeshadr <gi...@git.apache.org> on 2016/11/03 09:36:32 UTC
[GitHub] zookeeper pull request #98: ZOOKEEPER-2479
GitHub user rakeshadr opened a pull request:
https://github.com/apache/zookeeper/pull/98
ZOOKEEPER-2479
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rakeshadr/zookeeper-1 ZK-2479
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/98.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 #98
----
commit e4fe6cd5a1728c0847a392893b56860dbde1f7bd
Author: Rakesh Radhakrishnan <ra...@apache.org>
Date: 2016-11-03T09:33:03Z
ZOOKEEPER-2479
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #98: ZOOKEEPER-2479
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/98#discussion_r88604035
--- Diff: src/java/test/org/apache/zookeeper/test/QuorumMajorityTest.java ---
@@ -36,7 +38,30 @@
/***************************************************************/
@Test
public void testMajQuorums() throws Throwable {
-
+ LOG.info("Verify QuorumPeer#electionTimeTaken jmx bean attribute");
+
+ ArrayList<QuorumPeer> peers = getPeerList();
+ for (int i = 1; i <= peers.size(); i++) {
+ QuorumPeer qp = peers.get(i - 1);
+ Long electionTimeTaken = -1L;
+ String bean = "";
+ if (qp.getPeerState() == ServerState.FOLLOWING) {
+ bean = CommonNames.DOMAIN + ":name0=ReplicatedServer_id" + i
+ + ",name1=replica." + i + ",name2=Follower";
--- End diff --
Thanks @rgs1, I've have modified the test code as per your suggestion. Please take another look at the new commits.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #98: ZOOKEEPER-2479
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/98
@eribeiro ,, appreciate your feedback. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #98: ZOOKEEPER-2479
Posted by fpj <gi...@git.apache.org>.
Github user fpj commented on the issue:
https://github.com/apache/zookeeper/pull/98
+1, LGTM. @eribeiro is this ready according to you?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #98: ZOOKEEPER-2479
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/98#discussion_r86415195
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -520,6 +520,12 @@ public synchronized void setCurrentVote(Vote v){
protected boolean quorumListenOnAllIPs = false;
/**
+ * Keeps time taken for leader election in milliseconds. Sets the value to
+ * this variable only after the completion of leader election.
+ */
+ private long electionTimeTaken = -1;
--- End diff --
sincere question: does it make sense to make this field `volatile`? I know there are some concurrency guarantees but not sure if it's worth change it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #98: ZOOKEEPER-2479
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/98
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #98: ZOOKEEPER-2479
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/98#discussion_r86502042
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -520,6 +520,12 @@ public synchronized void setCurrentVote(Vote v){
protected boolean quorumListenOnAllIPs = false;
/**
+ * Keeps time taken for leader election in milliseconds. Sets the value to
+ * this variable only after the completion of leader election.
+ */
+ private long electionTimeTaken = -1;
--- End diff --
Yup, agree. :) Thanks for explaining!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #98: ZOOKEEPER-2479
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/98#discussion_r86483860
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java ---
@@ -520,6 +520,12 @@ public synchronized void setCurrentVote(Vote v){
protected boolean quorumListenOnAllIPs = false;
/**
+ * Keeps time taken for leader election in milliseconds. Sets the value to
+ * this variable only after the completion of leader election.
+ */
+ private long electionTimeTaken = -1;
--- End diff --
Thanks @eribeiro ,
LeaderMXBean, FollowerMXBean will be available only after the quorum leader election and the value won't be changed until next LE. I think, adding 'volatile' doesn't make any difference, right?
code reference:
https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/quorum/Leader.java#L417
https://github.com/apache/zookeeper/blob/master/src/java/main/org/apache/zookeeper/server/quorum/Follower.java#L70
Leader.java
zk.registerJMX(new LeaderBean(this, zk), self.jmxLocalPeerBean);
Follower.java
fzk.registerJMX(new FollowerBean(this, zk), self.jmxLocalPeerBean);
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #98: ZOOKEEPER-2479
Posted by rgs1 <gi...@git.apache.org>.
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/98#discussion_r88602850
--- Diff: src/java/test/org/apache/zookeeper/test/QuorumMajorityTest.java ---
@@ -36,7 +38,30 @@
/***************************************************************/
@Test
public void testMajQuorums() throws Throwable {
-
+ LOG.info("Verify QuorumPeer#electionTimeTaken jmx bean attribute");
+
+ ArrayList<QuorumPeer> peers = getPeerList();
+ for (int i = 1; i <= peers.size(); i++) {
+ QuorumPeer qp = peers.get(i - 1);
+ Long electionTimeTaken = -1L;
+ String bean = "";
+ if (qp.getPeerState() == ServerState.FOLLOWING) {
+ bean = CommonNames.DOMAIN + ":name0=ReplicatedServer_id" + i
+ + ",name1=replica." + i + ",name2=Follower";
+ electionTimeTaken = (Long) JMXEnv.ensureBeanAttribute(bean,
+ "ElectionTimeTaken");
+ Assert.assertTrue("Wrong electionTimeTaken value!",
+ electionTimeTaken >= 0);
+ } else if (qp.getPeerState() == ServerState.LEADING) {
+ bean = CommonNames.DOMAIN + ":name0=ReplicatedServer_id" + i
+ + ",name1=replica." + i + ",name2=Leader";
--- End diff --
ditto
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #98: ZOOKEEPER-2479
Posted by rgs1 <gi...@git.apache.org>.
Github user rgs1 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/98#discussion_r88602842
--- Diff: src/java/test/org/apache/zookeeper/test/QuorumMajorityTest.java ---
@@ -36,7 +38,30 @@
/***************************************************************/
@Test
public void testMajQuorums() throws Throwable {
-
+ LOG.info("Verify QuorumPeer#electionTimeTaken jmx bean attribute");
+
+ ArrayList<QuorumPeer> peers = getPeerList();
+ for (int i = 1; i <= peers.size(); i++) {
+ QuorumPeer qp = peers.get(i - 1);
+ Long electionTimeTaken = -1L;
+ String bean = "";
+ if (qp.getPeerState() == ServerState.FOLLOWING) {
+ bean = CommonNames.DOMAIN + ":name0=ReplicatedServer_id" + i
+ + ",name1=replica." + i + ",name2=Follower";
--- End diff --
hyper nit: `String.format()` reads better than `+`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---