zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From hanm <...@git.apache.org>
Subject [GitHub] zookeeper pull request #580: ZOOKEEPER-3098: Add additional server metrics
Date Mon, 10 Sep 2018 22:13:25 GMT
Github user hanm commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/580#discussion_r216489582
  
    --- Diff: src/java/main/org/apache/zookeeper/server/ServerStats.java ---
    @@ -33,17 +34,17 @@
     public class ServerStats {
         private static final Logger LOG = LoggerFactory.getLogger(ServerStats.class);
     
    -    private long packetsSent;
    -    private long packetsReceived;
    -    private long maxLatency;
    -    private long minLatency = Long.MAX_VALUE;
    -    private long totalLatency = 0;
    -    private long count = 0;
    +    private final AtomicLong packetsSent = new AtomicLong();
    --- End diff --
    
    Since `ServerMetrics` is a superset of `ServerStats` in terms of scope, we probably want
to keep `ServerStats` as is and ultimately deprecate it in favor of `ServerMetrics`. I don't
think there is a need to duplicate metrics in two places, which would be both a burden to
maintain and a potential source of confusion.
    
    Regarding reporting to `mntr`, we decided deprecate 4lw last year due to the limitation
of its design, in particular around security, in favor of admin server endpoints  (`/metrics`
in this case), so I don't there is a need to report newly added metrics to `mntr`. This also
encourages users to migrate away from 4lw to admin end points.
    
    Overall the state in current patch w.r.t this looks good to me.



---

Mime
View raw message