[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r163458731
  
--- Diff: src/java/main/org/apache/zookeeper/server/ZKDatabase.java ---
@@ -264,19 +262,8 @@ public void addCommittedProposal(Request request) {
 maxCommittedLog = request.zxid;
 }
 
-ByteArrayOutputStream baos = new ByteArrayOutputStream();
-BinaryOutputArchive boa = BinaryOutputArchive.getArchive(baos);
-try {
-request.getHdr().serialize(boa, "hdr");
-if (request.getTxn() != null) {
-request.getTxn().serialize(boa, "txn");
-}
-baos.close();
-} catch (IOException e) {
-LOG.error("This really should be impossible", e);
-}
-QuorumPacket pp = new QuorumPacket(Leader.PROPOSAL, 
request.zxid,
-baos.toByteArray(), null);
+byte[] data = SerializeUtils.serializeRequest(request);
--- End diff --

Just a small refactoring. I'm happy to submit in another PR.


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r163458563
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java ---
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.JmxReporter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import org.apache.zookeeper.jmx.CommonNames;
+import org.apache.zookeeper.jmx.MBeanRegistry;
+
+import static com.codahale.metrics.MetricRegistry.name;
+
+/**
+ * Provides real-time metrics on Leader's proposal size.
+ * The class uses a histogram included in Dropwizard metrics library with 
ExponentiallyDecayingReservoir.
+ * It provides stats of proposal sizes from the last 5 minutes with 
acceptable cpu/memory footprint optimized for streaming data.
+ */
+public class ProposalStats {
+private final Histogram proposalSizes;
+
+ProposalStats() {
+final MetricRegistry metrics = new MetricRegistry();
+Reservoir reservoir = new ExponentiallyDecayingReservoir();
--- End diff --


http://metrics.dropwizard.io/3.1.0/apidocs/com/codahale/metrics/ExponentiallyDecayingReservoir.html
Default constructor:
> Creates a new ExponentiallyDecayingReservoir of 1028 elements, which 
offers a 99.9% confidence level with a 5% margin of error assuming a normal 
distribution, and an alpha factor of 0.015, which heavily biases the reservoir 
to the past 5 minutes of measurements.
There're 2 other constructors which the reservoir can be configured with by 
size, alpha and clock. So, the window can be extended, though it's not as 
obvious as with `SlidingWindow`.

Let's go with that as you suggested.


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[phunt]

Github user phunt commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r163391127
  
--- Diff: src/java/main/org/apache/zookeeper/server/ZKDatabase.java ---
@@ -264,19 +262,8 @@ public void addCommittedProposal(Request request) {
 maxCommittedLog = request.zxid;
 }
 
-ByteArrayOutputStream baos = new ByteArrayOutputStream();
-BinaryOutputArchive boa = BinaryOutputArchive.getArchive(baos);
-try {
-request.getHdr().serialize(boa, "hdr");
-if (request.getTxn() != null) {
-request.getTxn().serialize(boa, "txn");
-}
-baos.close();
-} catch (IOException e) {
-LOG.error("This really should be impossible", e);
-}
-QuorumPacket pp = new QuorumPacket(Leader.PROPOSAL, 
request.zxid,
-baos.toByteArray(), null);
+byte[] data = SerializeUtils.serializeRequest(request);
--- End diff --

Is this really apropos to the stated reason for the PR? If not separating 
out to another PR means 1) easier to review this patch, and 2) might be easier 
to get this committed separately rather than tying it to another issue.


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[afine]

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r163390292
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java ---
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.JmxReporter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import org.apache.zookeeper.jmx.CommonNames;
+import org.apache.zookeeper.jmx.MBeanRegistry;
+
+import static com.codahale.metrics.MetricRegistry.name;
+
+/**
+ * Provides real-time metrics on Leader's proposal size.
+ * The class uses a histogram included in Dropwizard metrics library with 
ExponentiallyDecayingReservoir.
+ * It provides stats of proposal sizes from the last 5 minutes with 
acceptable cpu/memory footprint optimized for streaming data.
+ */
+public class ProposalStats {
+private final Histogram proposalSizes;
+
+ProposalStats() {
+final MetricRegistry metrics = new MetricRegistry();
+Reservoir reservoir = new ExponentiallyDecayingReservoir();
--- End diff --

From: 
http://metrics.dropwizard.io/3.1.0/manual/core/#exponentially-decaying-reservoirs

> A histogram with an exponentially decaying reservoir produces quantiles 
which are representative of (roughly) the last five minutes of data.

My guess is that we would want more than data representing the last 5 
minutes right?


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r161661577
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java ---
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.JmxReporter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import org.apache.zookeeper.jmx.CommonNames;
+import org.apache.zookeeper.jmx.MBeanRegistry;
+
+import static com.codahale.metrics.MetricRegistry.name;
+
+/**
+ * Provides real-time metrics on Leader's proposal size.
+ * The class uses a histogram included in Dropwizard metrics library with 
ExponentiallyDecayingReservoir.
+ * It provides stats of proposal sizes from the last 5 minutes with 
acceptable cpu/memory footprint optimized for streaming data.
+ */
+public class ProposalStats {
+private final Histogram proposalSizes;
+
+ProposalStats() {
+final MetricRegistry metrics = new MetricRegistry();
+Reservoir reservoir = new ExponentiallyDecayingReservoir();
--- End diff --

@afine Would you approve the PR with existing reservoir or do you still 
think it'd be better to go with configurable sliding window?


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160891125
  
--- Diff: build.xml ---
@@ -198,7 +198,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
 
 
 
-
+
--- End diff --

Great. Works.


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[afine]

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160889741
  
--- Diff: build.xml ---
@@ -198,7 +198,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
 
 
 
-
+
--- End diff --

Can we just exclude org.slf4j#slf4j-api;1.7.22 ?


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160689248
  
--- Diff: build.xml ---
@@ -198,7 +198,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
 
 
 
-
+
--- End diff --

Here's the issue:
{code}
BUILD FAILED
/Users/Molnar/git/my-zookeeper/build.xml:420: impossible to resolve 
dependencies:
org.slf4j#slf4j-api;1.7.22 (needed by 
[io.dropwizard.metrics#metrics-core;3.2.5]) conflicts with 
org.slf4j#slf4j-api;1.7.25 (needed by 
[org.apache.zookeeper#zookeeper;3.6.0-SNAPSHOT])
{code}


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160524365
  
--- Diff: build.xml ---
@@ -198,7 +198,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
 
 
 
-
+
--- End diff --

yes, to be consistent with slf4j


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[afine]

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160483835
  
--- Diff: build.xml ---
@@ -198,7 +198,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
 
 
 
-
+
--- End diff --

So do we still need this lower version?


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160422407
  
--- Diff: 
src/java/test/org/apache/zookeeper/server/quorum/LeaderBeanTest.java ---
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import org.apache.jute.OutputArchive;
--- End diff --

thanks, done.


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160422461
  
--- Diff: 
src/java/test/org/apache/zookeeper/server/quorum/LeaderBeanTest.java ---
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import org.apache.jute.OutputArchive;
+import org.apache.jute.Record;
+import org.apache.zookeeper.server.Request;
+import org.apache.zookeeper.server.ZKDatabase;
+import org.apache.zookeeper.server.persistence.FileTxnSnapLog;
+import org.apache.zookeeper.server.quorum.flexible.QuorumVerifier;
+import org.apache.zookeeper.server.util.SerializeUtils;
+import org.apache.zookeeper.test.ClientBase;
+import org.apache.zookeeper.txn.TxnHeader;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+import java.io.File;
+import java.io.IOException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+
+public class LeaderBeanTest {
+private Leader leader;
+private LeaderBean leaderBean;
+private FileTxnSnapLog fileTxnSnapLog;
+private LeaderZooKeeperServer zks;
+private QuorumPeer qp;
+
+@Before
+public void setUp() throws IOException {
+qp = new QuorumPeer();
+QuorumVerifier quorumVerifierMock = mock(QuorumVerifier.class);
+qp.setQuorumVerifier(quorumVerifierMock, false);
+File tmpDir = ClientBase.createEmptyTestDir();
+fileTxnSnapLog = new FileTxnSnapLog(new File(tmpDir, "data"),
+new File(tmpDir, "data_txnlog"));
+ZKDatabase zkDb = new ZKDatabase(fileTxnSnapLog);
+
+zks = new LeaderZooKeeperServer(fileTxnSnapLog, qp, zkDb);
+leader = new Leader(qp, zks);
+leaderBean = new LeaderBean(leader, zks);
+}
+
+@After
+public void tearDown() throws IOException {
+fileTxnSnapLog.close();
+}
+
+@Test
+public void testGetName() {
+assertEquals("Leader", leaderBean.getName());
+}
+
+@Test
+public void testGetCurrentZxid() {
+// Arrange
+zks.setZxid(1);
+
+// Assert
+assertEquals("0x1", leaderBean.getCurrentZxid());
+}
+
+@Test
+public void testGetElectionTimeTaken() {
+// Arrange
+qp.setElectionTimeTaken(1);
+
+// Assert
+assertEquals(1, leaderBean.getElectionTimeTaken());
+}
+
+private Request createMockRequest() throws IOException {
--- End diff --

No, removed.


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160401512
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java ---
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.JmxReporter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import org.apache.zookeeper.jmx.CommonNames;
+import org.apache.zookeeper.jmx.MBeanRegistry;
+
+import static com.codahale.metrics.MetricRegistry.name;
+
+/**
+ * Provides real-time metrics on Leader's proposal size.
+ * The class uses a histogram included in Dropwizard metrics library with 
ExponentiallyDecayingReservoir.
+ * It provides stats of proposal sizes from the last 5 minutes with 
acceptable cpu/memory footprint optimized for streaming data.
+ */
+public class ProposalStats {
+private final Histogram proposalSizes;
+
+ProposalStats() {
+final MetricRegistry metrics = new MetricRegistry();
+Reservoir reservoir = new ExponentiallyDecayingReservoir();
--- End diff --

I'm not an expert either. According to the following article I think we've 
the following reservoir options:
http://taint.org/2014/01/16/145944a.html

SlidingTimeWindowReservoir: the standard one, but looks like the memory 
footprint could be potentially high in some cases.
SlidingTimeWindowArrayReservoir: similar to the previous with much better 
memory handling
ExponentiallyDecayingReservoir: good memory/cpu footprint and optimised for 
streaming data. The main difference is that if there's no data comes in within 
the configured number of samples, previously collected statistics will be 
presented on JMX. SlidingTimeWindow reservoirs will zero out, which sounds 
correct behaviour to me, but it might be less convenient.



---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160399269
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java ---
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
--- End diff --

Doesn't look like it has separate package for that.


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160398954
  
--- Diff: ivy.xml ---
@@ -133,6 +133,12 @@
 
 
+
+
+

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

Github user anmolnar commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160398663
  
--- Diff: build.xml ---
@@ -198,7 +198,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
 
 
 
-
+
--- End diff --

Not sure how to get an evidence, but if remember correctly it was 
dropwizard-core which forced me to use an earlier version of slf4j.


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[afine]

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160233203
  
--- Diff: ivy.xml ---
@@ -133,6 +133,12 @@
 
 
+
+
+

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[afine]

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r159986909
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java ---
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
--- End diff --

I don't see much being pulled in from dropwizard, only codahale. Is 
codahale.metrics available independently? 

It doesn't look like it but I only checked briefly.
  


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[afine]

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160236436
  
--- Diff: 
src/java/test/org/apache/zookeeper/server/quorum/LeaderBeanTest.java ---
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import org.apache.jute.OutputArchive;
+import org.apache.jute.Record;
+import org.apache.zookeeper.server.Request;
+import org.apache.zookeeper.server.ZKDatabase;
+import org.apache.zookeeper.server.persistence.FileTxnSnapLog;
+import org.apache.zookeeper.server.quorum.flexible.QuorumVerifier;
+import org.apache.zookeeper.server.util.SerializeUtils;
+import org.apache.zookeeper.test.ClientBase;
+import org.apache.zookeeper.txn.TxnHeader;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+import java.io.File;
+import java.io.IOException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+
+public class LeaderBeanTest {
+private Leader leader;
+private LeaderBean leaderBean;
+private FileTxnSnapLog fileTxnSnapLog;
+private LeaderZooKeeperServer zks;
+private QuorumPeer qp;
+
+@Before
+public void setUp() throws IOException {
+qp = new QuorumPeer();
+QuorumVerifier quorumVerifierMock = mock(QuorumVerifier.class);
+qp.setQuorumVerifier(quorumVerifierMock, false);
+File tmpDir = ClientBase.createEmptyTestDir();
+fileTxnSnapLog = new FileTxnSnapLog(new File(tmpDir, "data"),
+new File(tmpDir, "data_txnlog"));
+ZKDatabase zkDb = new ZKDatabase(fileTxnSnapLog);
+
+zks = new LeaderZooKeeperServer(fileTxnSnapLog, qp, zkDb);
+leader = new Leader(qp, zks);
+leaderBean = new LeaderBean(leader, zks);
+}
+
+@After
+public void tearDown() throws IOException {
+fileTxnSnapLog.close();
+}
+
+@Test
+public void testGetName() {
+assertEquals("Leader", leaderBean.getName());
+}
+
+@Test
+public void testGetCurrentZxid() {
+// Arrange
+zks.setZxid(1);
+
+// Assert
+assertEquals("0x1", leaderBean.getCurrentZxid());
+}
+
+@Test
+public void testGetElectionTimeTaken() {
+// Arrange
+qp.setElectionTimeTaken(1);
+
+// Assert
+assertEquals(1, leaderBean.getElectionTimeTaken());
+}
+
+private Request createMockRequest() throws IOException {
--- End diff --

is this ever used?


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[afine]

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r159987502
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java ---
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.JmxReporter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import org.apache.zookeeper.jmx.CommonNames;
+import org.apache.zookeeper.jmx.MBeanRegistry;
+
+import static com.codahale.metrics.MetricRegistry.name;
+
+/**
+ * Provides real-time metrics on Leader's proposal size.
+ * The class uses a histogram included in Dropwizard metrics library with 
ExponentiallyDecayingReservoir.
+ * It provides stats of proposal sizes from the last 5 minutes with 
acceptable cpu/memory footprint optimized for streaming data.
+ */
+public class ProposalStats {
+private final Histogram proposalSizes;
+
+ProposalStats() {
+final MetricRegistry metrics = new MetricRegistry();
+Reservoir reservoir = new ExponentiallyDecayingReservoir();
--- End diff --

I won't pretend to know much about exponentially decaying reservoirs. I'm 
curious what the behavior is with minimum and maximum values. Are these 
guaranteed to always be exact and for what time period?


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[afine]

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r159985176
  
--- Diff: build.xml ---
@@ -198,7 +198,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
 
 
 
-
+
--- End diff --

what is the reason for the downgrade?


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[afine]

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160236050
  
--- Diff: 
src/java/test/org/apache/zookeeper/server/quorum/LeaderBeanTest.java ---
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import org.apache.jute.OutputArchive;
--- End diff --

nit: there are a couple unused imports here


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[afine]

Github user afine commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/440#discussion_r160249680
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/quorum/ProposalStats.java ---
@@ -0,0 +1,70 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server.quorum;
+
+import com.codahale.metrics.ExponentiallyDecayingReservoir;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.JmxReporter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Reservoir;
+import com.codahale.metrics.Snapshot;
+import org.apache.zookeeper.jmx.CommonNames;
+import org.apache.zookeeper.jmx.MBeanRegistry;
+
+import static com.codahale.metrics.MetricRegistry.name;
+
+/**
+ * Provides real-time metrics on Leader's proposal size.
+ * The class uses a histogram included in Dropwizard metrics library with 
ExponentiallyDecayingReservoir.
+ * It provides stats of proposal sizes from the last 5 minutes with 
acceptable cpu/memory footprint optimized for streaming data.
+ */
+public class ProposalStats {
+private final Histogram proposalSizes;
+
+ProposalStats() {
+final MetricRegistry metrics = new MetricRegistry();
+Reservoir reservoir = new ExponentiallyDecayingReservoir();
--- End diff --

Perhaps a user configurable SlidingTimeWindowReservoir may be more 
appropriate?


---

[GitHub] zookeeper pull request #440: ZOOKEEPER-2939 Deal with maxbuffer as it relate...

[anmolnar]

GitHub user anmolnar opened a pull request:

https://github.com/apache/zookeeper/pull/440

ZOOKEEPER-2939 Deal with maxbuffer as it relates to proposals - Use 
dropwizard stats library

This PR is intended to be the successor of 
https://github.com/apache/zookeeper/pull/415
Using dropwizard library's Histogram component we're able to provide more 
sophisticated statistics on Proposal sizes. 

From the docs:
"A histogram measures the statistical distribution of values in a stream of 
data. In addition to minimum, maximum, mean, etc., it also measures median, 
75th, 90th, 95th, 98th, 99th, and 99.9th percentiles."

http://metrics.dropwizard.io/3.1.0/manual/core/#histograms


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/anmolnar/zookeeper ZOOKEEPER-2939-dropwizard

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/440.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 #440


commit 6738e2d36b07f4643a97afd7e251a9d6c1c115f2
Author: Andor Molnár 
Date:   2017-11-06T16:31:44Z

ZOOKEEPER-2939: Added last/min/max proposal size JMX beans

- Refactor proposal serialization logic to a common place,
- Add JMX metric to monitor jute.maxbuffer setting
- Add JMX metric to monitor min/max/last size of generated proposals
- This patch deals with server side maxbuffer monitoring only.

commit 68328d9027bf2a26d3425b9c83b4a9d5a526fc9b
Author: Andor Molnár 
Date:   2017-12-01T11:59:48Z

ZOOKEEPER-2939. Fixed findbugs issue: newline in format string

commit ece8f09cb09c456ccb77903c9a2f6977c34c1543
Author: Andor Molnar 
Date:   2017-12-15T18:11:17Z

ZOOKEEPER-2933. Added proposal size statistics to 'mntr' command

commit 89376898995ecb60e6f26252853fce1d6d85ba2b
Author: Andor Molnar 
Date:   2017-12-18T16:34:24Z

ZOOKEEPER-2939. Added dropwizard library to calculate histogram of proposal 
sizes

commit 70faba6c8f7478d44edf5fd22557df533bbbc6ce
Author: Andor Molnar 
Date:   2018-01-02T15:24:12Z

ZOOKEEPER-2939. Fixed/added unit tests for Stat and Monitor commands




---