[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[anmolnar]

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

https://github.com/apache/zookeeper/pull/466#discussion_r183664859
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1897,6 +1896,12 @@ server.3=zoo3:2888:3888
   zk_pending_syncs0   - only exposed by the 
Leader
   zk_open_file_descriptor_count 23- only available on Unix 
platforms
   zk_max_file_descriptor_count 1024   - only available on Unix 
platforms
+  zk_last_proposal_size
--- End diff --

Correct, I'll add some example values.


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[anmolnar]

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

https://github.com/apache/zookeeper/pull/466#discussion_r183664194
  
--- Diff: src/contrib/monitoring/ganglia/zookeeper_ganglia.py ---
@@ -213,7 +213,13 @@ def metric_init(params=None):
 'zk_max_file_descriptor_count': {'units': 'descriptors'},
 'zk_followers': {'units': 'nodes'},
 'zk_synced_followers': {'units': 'nodes'},
-'zk_pending_syncs': {'units': 'syncs'}
+'zk_pending_syncs': {'units': 'syncs'},
+'zk_last_proposal_size': {'units': 'ms'},
--- End diff --

Right. Good catch.


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[phunt]

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

https://github.com/apache/zookeeper/pull/466#discussion_r183565978
  
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -1897,6 +1896,12 @@ server.3=zoo3:2888:3888
   zk_pending_syncs0   - only exposed by the 
Leader
   zk_open_file_descriptor_count 23- only available on Unix 
platforms
   zk_max_file_descriptor_count 1024   - only available on Unix 
platforms
+  zk_last_proposal_size
--- End diff --

these are missing example values


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[phunt]

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

https://github.com/apache/zookeeper/pull/466#discussion_r183565848
  
--- Diff: src/contrib/monitoring/ganglia/zookeeper_ganglia.py ---
@@ -213,7 +213,13 @@ def metric_init(params=None):
 'zk_max_file_descriptor_count': {'units': 'descriptors'},
 'zk_followers': {'units': 'nodes'},
 'zk_synced_followers': {'units': 'nodes'},
-'zk_pending_syncs': {'units': 'syncs'}
+'zk_pending_syncs': {'units': 'syncs'},
+'zk_last_proposal_size': {'units': 'ms'},
--- End diff --

Should these values be in "ms"? Seems like a cut/paste typo? I suspect you 
rather "bytes" - right?


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/466#discussion_r183201618
  
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Leader.java ---
@@ -106,10 +104,10 @@ public String toString() {
 private final HashSet learners =
 new HashSet();
 
-private final ProposalStats proposalStats;
+private final BufferStats bufferStats;
--- End diff --

Could you use "proposalStats" variable name instead of "bufferStats", thats 
more readable.


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/466#discussion_r183201663
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/StatCommand.java ---
@@ -64,8 +64,8 @@ public void commandRun() {
 pw.println(zkServer.getZKDatabase().getNodeCount());
 if (serverStats.getServerState().equals("leader")) {
 Leader leader = 
((LeaderZooKeeperServer)zkServer).getLeader();
-ProposalStats proposalStats = leader.getProposalStats();
-pw.printf("Proposal sizes last/min/max: %s%n", 
proposalStats.toString());
+BufferStats bufferStats = leader.getBufferStats();
--- End diff --

Same as above. Please use var name "proposalStats" instead of "bufferStats"


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[anmolnar]

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

https://github.com/apache/zookeeper/pull/466#discussion_r182698169
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/MonitorCommand.java ---
@@ -75,9 +79,9 @@ public void commandRun() {
 print("synced_followers", 
leader.getForwardingFollowers().size());
 print("pending_syncs", leader.getNumPendingSyncs());
 
-print("last_proposal_size", 
leader.getProposalStats().getLastProposalSize());
-print("max_proposal_size", 
leader.getProposalStats().getMaxProposalSize());
-print("min_proposal_size", 
leader.getProposalStats().getMinProposalSize());
+print("last_proposal_size", 
leader.getProposalStats().getLast());
--- End diff --

You're right about stats will evolve with time, but proposal and client 
request size are common in Jute buffer usage. I have a strong feeling that 
these two things will always evolve together hence it makes sense to keep the 
stats together which also makes refactoring easier. Later, if it turns out that 
they diverge, we can easily split them. 


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[anmolnar]

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

https://github.com/apache/zookeeper/pull/466#discussion_r182459083
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/MonitorCommand.java ---
@@ -75,9 +79,9 @@ public void commandRun() {
 print("synced_followers", 
leader.getForwardingFollowers().size());
 print("pending_syncs", leader.getNumPendingSyncs());
 
-print("last_proposal_size", 
leader.getProposalStats().getLastProposalSize());
-print("max_proposal_size", 
leader.getProposalStats().getMaxProposalSize());
-print("min_proposal_size", 
leader.getProposalStats().getMinProposalSize());
+print("last_proposal_size", 
leader.getProposalStats().getLast());
--- End diff --

Agreed, javadoc is misleading.

Actually I wanted to keep these two things together and would like to 
refactor both in a later commit. They refer to the same thing basically, 
exposing statistics on Jute buffer usage which is a feed of int values. I'm 
happy to find a better name for the class, but wouldn't create a separate one 
for almost the same purpose.

What do you think of `BufferStats`?




---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/466#discussion_r182332830
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/MonitorCommand.java ---
@@ -75,9 +79,9 @@ public void commandRun() {
 print("synced_followers", 
leader.getForwardingFollowers().size());
 print("pending_syncs", leader.getNumPendingSyncs());
 
-print("last_proposal_size", 
leader.getProposalStats().getLastProposalSize());
-print("max_proposal_size", 
leader.getProposalStats().getMaxProposalSize());
-print("min_proposal_size", 
leader.getProposalStats().getMinProposalSize());
+print("last_proposal_size", 
leader.getProposalStats().getLast());
--- End diff --

One more observation. It seems "ProposalStats" is reused for 
clientResponseStats metrics, but it may create confusions due to the method 
name mismatches. Again, ProposalStats javadocs says "Provides live statistics 
about a running Leader."  Also, stats will evolve and could be chance of adding 
unrelated metrics later. 

How about create a new class "ResponseStats" and name metrics like below. 
If we look at the existing min_proposal_size metrics, they didn't use the term 
leader/quorum. Keeping that in mind, do we need specifically ''client'' term in 
the metrics, simply response gives a context to the users that server-to-client 
response. Whats your opinion?
```
last_response_size,
max_response_size,
min_response_size
```


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/466#discussion_r182325325
  
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java 
---
@@ -68,5 +74,19 @@ public void testOperationsAfterCnxnClose() throws 
IOException,
 } finally {
 zk.close();
 }
+
+}
+
+@Test
+public void testClientResponseStatsUpdate() throws IOException, 
InterruptedException, KeeperException {
+try (ZooKeeper zk = createClient()) {
+ProposalStats stats = 
serverFactory.getZooKeeperServer().serverStats().getClientResponseStats();
+assertEquals("", -1, stats.getLast());
+
+zk.create("/a", "test".getBytes(), Ids.OPEN_ACL_UNSAFE,
+CreateMode.PERSISTENT);
+
+assertThat(stats.getLast(), greaterThan(0));
--- End diff --

please add comments about the expectations like, 
assertEquals("stat should be initialized with -1", -1, stats.getLast());  
or a better message:)

that will help easy test case maintenance.
b) greaterThan(0) , similarly good to add a message explaining why 0?


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[anmolnar]

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

https://github.com/apache/zookeeper/pull/466#discussion_r182196539
  
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java 
---
@@ -68,5 +74,19 @@ public void testOperationsAfterCnxnClose() throws 
IOException,
 } finally {
 zk.close();
 }
+
+}
+
+@Test
+public void testClientResponseStatsUpdate() throws IOException, 
InterruptedException, KeeperException {
+try (ZooKeeper zk = createClient()) {
+ProposalStats stats = 
serverFactory.getZooKeeperServer().serverStats().getClientResponseStats();
+assertEquals("", -1, stats.getLast());
+
+zk.create("/a", "test".getBytes(), Ids.OPEN_ACL_UNSAFE,
+CreateMode.PERSISTENT);
+
+assertThat(stats.getLast(), greaterThan(0));
--- End diff --

+1 for assert messages
Did you mean -1 as the magic number which means that stat has not been 
updated yet...?


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[anmolnar]

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

https://github.com/apache/zookeeper/pull/466#discussion_r182196063
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/MonitorCommand.java ---
@@ -75,9 +79,9 @@ public void commandRun() {
 print("synced_followers", 
leader.getForwardingFollowers().size());
 print("pending_syncs", leader.getNumPendingSyncs());
 
-print("last_proposal_size", 
leader.getProposalStats().getLastProposalSize());
-print("max_proposal_size", 
leader.getProposalStats().getMaxProposalSize());
-print("min_proposal_size", 
leader.getProposalStats().getMinProposalSize());
+print("last_proposal_size", 
leader.getProposalStats().getLast());
--- End diff --

Sure, no problem. I thought it was redundant to refer the class name in 
method names, but I'll revert that.


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/466#discussion_r181829051
  
--- Diff: src/java/main/org/apache/zookeeper/server/admin/Commands.java ---
@@ -329,12 +329,20 @@ public CommandResponse run(ZooKeeperServer zkServer, 
Map kwargs)
 response.put("open_file_descriptor_count", 
osMbean.getOpenFileDescriptorCount());
 response.put("max_file_descriptor_count", 
osMbean.getMaxFileDescriptorCount());
 
+response.put("last_client_response_size", 
stats.getClientResponseStats().getLast());
--- End diff --

Also, I could see contrib/monitoring/ganglia/zookeeper_ganglia.py file 
contains metrics section. Probably you could refer 
'zk_open_file_descriptor_count': {'units': 'descriptors'}, as an example case. 


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/466#discussion_r181817033
  
--- Diff: 
src/java/main/org/apache/zookeeper/server/command/MonitorCommand.java ---
@@ -75,9 +79,9 @@ public void commandRun() {
 print("synced_followers", 
leader.getForwardingFollowers().size());
 print("pending_syncs", leader.getNumPendingSyncs());
 
-print("last_proposal_size", 
leader.getProposalStats().getLastProposalSize());
-print("max_proposal_size", 
leader.getProposalStats().getMaxProposalSize());
-print("min_proposal_size", 
leader.getProposalStats().getMinProposalSize());
+print("last_proposal_size", 
leader.getProposalStats().getLast());
--- End diff --

Infact, getLastProposalSize, getMaxProposalSize, getMinProposalSize is more 
meaningful. Can we keep this names?


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/466#discussion_r181824379
  
--- Diff: src/java/main/org/apache/zookeeper/server/admin/Commands.java ---
@@ -329,12 +329,20 @@ public CommandResponse run(ZooKeeperServer zkServer, 
Map kwargs)
 response.put("open_file_descriptor_count", 
osMbean.getOpenFileDescriptorCount());
 response.put("max_file_descriptor_count", 
osMbean.getMaxFileDescriptorCount());
 
+response.put("last_client_response_size", 
stats.getClientResponseStats().getLast());
--- End diff --

Please update zookeeperAdmin.xml about these commands. Please refer 
monitoring the health of the cluster section in this page.



---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/466#discussion_r181822557
  
--- Diff: src/java/test/org/apache/zookeeper/server/NIOServerCnxnTest.java 
---
@@ -68,5 +74,19 @@ public void testOperationsAfterCnxnClose() throws 
IOException,
 } finally {
 zk.close();
 }
+
+}
+
+@Test
+public void testClientResponseStatsUpdate() throws IOException, 
InterruptedException, KeeperException {
+try (ZooKeeper zk = createClient()) {
+ProposalStats stats = 
serverFactory.getZooKeeperServer().serverStats().getClientResponseStats();
+assertEquals("", -1, stats.getLast());
+
+zk.create("/a", "test".getBytes(), Ids.OPEN_ACL_UNSAFE,
+CreateMode.PERSISTENT);
+
+assertThat(stats.getLast(), greaterThan(0));
--- End diff --

0, looks like magic number. Any possibility to give a non-zero value, 
matching closer to the expected value ?

Also, please add error message - assertThat("error message", 
stats.getLast(), greaterThan(0));



---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[rakeshadr]

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

https://github.com/apache/zookeeper/pull/466#discussion_r181823105
  
--- Diff: 
src/java/test/org/apache/zookeeper/server/NettyServerCnxnTest.java ---
@@ -84,4 +92,17 @@ public void testSendCloseSession() throws Exception {
 zk.close();
 }
 }
+
+@Test
+public void testClientResponseStatsUpdate() throws IOException, 
InterruptedException, KeeperException {
+try (ZooKeeper zk = createClient()) {
+ProposalStats stats = 
serverFactory.getZooKeeperServer().serverStats().getClientResponseStats();
+assertEquals("", -1, stats.getLast());
+
+zk.create("/a", "test".getBytes(), Ids.OPEN_ACL_UNSAFE,
+CreateMode.PERSISTENT);
+
+assertThat(stats.getLast(), greaterThan(0));
--- End diff --

Same comment as above...non-zero expected value.


---

[GitHub] zookeeper pull request #466: ZOOKEEPER-2940. Deal with maxbuffer as it relat...

[anmolnar]

GitHub user anmolnar opened a pull request:

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

ZOOKEEPER-2940. Deal with maxbuffer as it relates to large requests from 
clients

This PR covers the other part of Jute buffer monitoring which relates to 
client response sizes.

`jute.maxbuffer` is often set too small or the default (4MB) value is not 
enough to receive large responses from the server which causes the client 
unable to operate. e.g. `getChildren()` on a node which has lots of children or 
execution of a large multi.

These new metrics lets operators to monitor the size of generated responses 
to get an idea on how to properly set client's `jute.maxbuffer`.

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

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

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

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


commit 003ea900ea06e0aa763f7d8c71b6cd3859ac18b1
Author: Andor Molnar 
Date:   2018-02-13T17:04:12Z

ZOOKEEPER-2940. Track size of client responses




---