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

    https://github.com/apache/zookeeper/pull/307#discussion_r191201829
  
    --- Diff: src/java/main/org/apache/zookeeper/server/ServerStats.java ---
    @@ -106,6 +126,7 @@ public String toString(){
             StringBuilder sb = new StringBuilder();
             sb.append("Latency min/avg/max: " + getMinLatency() + "/"
                     + getAvgLatency() + "/" + getMaxLatency() + "\n");
    +        sb.append("Num Requests that exceeded threshold latency: " + 
getNumRequestsAboveThresholdTime() + "\n");
    --- End diff --
    
    This is going to be printed in `stat` 4lw command which makes things a 
little bit complicated.
    
    - what's the target branch/version of this patch? As we're in the middle of 
3.5 stabilisation, I would suggest to target this trunk/3.6-only. Please let me 
know if you've concerns and please update the Jira fix version accordingly.
    - Output of 4lw commands are parsed by machines occasionally, so adding new 
fields in the middle of the ouput would potentially brake backward 
compatibility. Adding them to the end of the output could be cumbersome in this 
case, but:
    - 4LWs have some security flaws and we should decommission them in the 
upcoming ZooKeeper versions in favour of JMX and Jetty interfaces,
    - I recommend not adding the new metric here, but expose it via JMX and 
Jetty `stat` and `monitor` commands.



---

Reply via email to