[
https://issues.apache.org/jira/browse/ZOOKEEPER-2290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14952676#comment-14952676
]
Edward Ribeiro commented on ZOOKEEPER-2290:
-------------------------------------------
Hi [~liushaohui], thanks for the patch. Below are some review points I raised:
1. {{updateLatency(long requestCreateTime, boolean isWrite, boolean isRead)}}
call with a lot of true/false, false/true is confusing, imo. This code can be
improved if you create an enum, inside {{ServerStats}}, as below:
{code}
public enum Ops {
READ, WRITE
}
{code}
then you can change the method signature to {{updateLatency(long
requestCreateTime, ServerStats.Ops op)}}. This improves readability because
each call will be {{updateLatency(request.createTime, ServerStas.Ops.WRITE)}}.
Also, making it an enum simplifies the body of {{updateLatency}} as below:
{code}
count.inc();
switch (Ops) {
case WRITE: writeCount.inc(); break;
case READ: readCount.inc(); break;
}
{code}
Finally, instead of repeating the
{{zks.serverStats().updateLatency(request.createTime, true, false)}} for each
possibility in {{FinalRequestProcessor}}, you can use a variable
{{ServerStats.StatsOp op = null;}} around line 158 and setting it up the value
of the variable where you previously call {{updateLatency()}}. Finally restore
the line 489 that you removed as
{{zks.serverStats().updateLatency(request.createTime, op)}}. TL;DR: no need to
repeat the updateLatency statement +20 times when you can use a variable and
call it at the end, as it is currently is called.
2. The class {{Rate}} is in fact a {{Counter}} or {{Gauge}} that has an extra
rate() method, so a name change would be more appropriate, imo.
3. Yet on the {{Rate}} class, please don't just throw in literals like 10000.
Give preference to define them as a static final constant fields with a proper
name (a single line commenting the relationale of why 10 seconds would be nice
too, imo).
4. I think it could be very nice if we could expose read/write ops via JMX too.
6. Finally, the patch is lacking tests.
> Add read/write qps metrics in monitor cmd
> -----------------------------------------
>
> Key: ZOOKEEPER-2290
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2290
> Project: ZooKeeper
> Issue Type: Improvement
> Affects Versions: 3.4.6
> Reporter: Liu Shaohui
> Priority: Minor
> Labels: monitor
> Fix For: 3.6.0
>
> Attachments: ZOOKEEPER-2290-v1.patch
>
>
> Read/write qps are important metrics to show the pressure of the cluster. We
> can also use it to alert about some abuse of zookeeper.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
