[
https://issues.apache.org/jira/browse/HBASE-16549?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15989463#comment-15989463
]
Appy commented on HBASE-16549:
------------------------------
Everything is already great! I won't mind committing v2 since it not only good
correctness wise, but uses new metrics api too!
Few design suggestions clicked when reviewing, so here they are. If any thing
doesn't seem reasonable, feel free to skip.
Apologies for late review, but reviewing raw patches and copy pasting stuff to
jira, puts me off. :)
-------------
OperationMetricsImpl can be make read-only object.
If we add a constructor to init metrices, {{OperationMetricsImpl(registry,
metricNamePrefix)}}, that'll
- get rid of repetitive - new followed by 3 set*() methods
- members can be made final.
- can remove 12 variables ( *_SUBMITTED_COUNT_NAME and others)
--------------
I believe you are putting OperationMetrics in BaseSource so both AM and Master
can use it. Maybe, we should move it out to separate class because BaseSource
is about types of metrics - counter, hist, guage (bare bones); rather than
their use (OperationMetrics has 'submitted', 'failed', and 'time' which doesn't
below to core metrics' classes). Maybe get rid of interface/impl and make it
single read-only class with final public members. (If pure data classes can be
made read-only, that's amazing and means that design was well thought!)
--------------
Functions with lots of return points are not good. We should change the
following pattern:
{noformat}
303 if (timeHisto == null) {
304 return;
305 }
306 timeHisto.update(runtime);
---
if (timeHisto != null) {
timeHisto.update(runtime);
}
{noformat}
--------------
{{ * @return Master's instnace of {@link MetricsMaster\}}}
typo
--------------
In AssignProcedure and other implementations, we can add a {{static
ProcedureMetric metric}} and only set it once. That also solidifies the idea
that these metrics are being aggregated by 'type-of-procedure'.
No need to keep the copy in hbase-server/..../Metrics*.java files then.
--------------
{{convertToProcedureMetrics}}
good one
--------------
> Procedure v2 - Add new AM metrics
> ---------------------------------
>
> Key: HBASE-16549
> URL: https://issues.apache.org/jira/browse/HBASE-16549
> Project: HBase
> Issue Type: Sub-task
> Components: proc-v2, Region Assignment
> Affects Versions: 2.0.0
> Reporter: Matteo Bertozzi
> Assignee: Umesh Agashe
> Fix For: 2.0.0
>
> Attachments: HBASE-16549-hbase-14614.v1.patch,
> HBASE-16549-hbase-14614.v2.patch
>
>
> With the new AM we can add a bunch of metrics
> - assign/unassign time
> - server crash time
> - grouping related metrics? (how many batch we do, and similar?)
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
