hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Appy (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-16549) Procedure v2 - Add new AM metrics
Date Fri, 28 Apr 2017 21:12:04 GMT

    [ 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)}},
- 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:
303	      if (timeHisto == null) {
304	        return;
305	      }
306	      timeHisto.update(runtime);
if (timeHisto != null) {
{{   * @return Master's instnace of {@link MetricsMaster\}}}
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.
 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

View raw message