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.
---
