[ 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)