[ https://issues.apache.org/jira/browse/HBASE-18409?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16129837#comment-16129837 ]
Josh Elser commented on HBASE-18409: ------------------------------------ Hi, [~macmaster]! Mighty nice patch you've put together here. Isolating the metrics aggregation behind the new hbase-metrics API is a great idea, IMO. {code} + /////// Other Bases /////// ... + /////// Other Bases /////// {code} Maybe tone these down? :) Just a leading comment character would be sufficient {code} + this.regionStats = new HashMap<String, RegionStats>(); + this.cacheDroppingExceptions = new HashMap<String, MutableFastCounter>(); {code} You can omit the types on the RHS. The bare diamond operator is sufficient. {code} + @VisibleForTesting + protected final Map<String, RegionStats> regionStats; + @VisibleForTesting + protected final Map<String, MutableFastCounter> cacheDroppingExceptions; {code} These were ConcurrentHashMaps above. Are we sure that we don't need the concurrency-safety now? {code} + private final String POOL_EXECUTOR_ACTIVE_KEY = "executorPoolActiveThreads"; + private final String POOL_EXECUTOR_ACTIVE_DESC = "number of active threads in the executor pool"; + private final String POOL_META_ACTIVE_KEY = "metaPoolActiveThreads"; + private final String POOL_META_ACTIVE_DESC = "number of active threads in the meta pool"; {code} These are net-new metrics? Should they be spun out into a different JIRA issue (we try to limit one "thing" per JIRA issue). FYI [~stack], I think you had done a review on the hbase-metrics API patch when Enis brought it in. Thought you might be interested in this one too. > Migrate Client Metrics from codahale to hbase-metrics > ----------------------------------------------------- > > Key: HBASE-18409 > URL: https://issues.apache.org/jira/browse/HBASE-18409 > Project: HBase > Issue Type: Improvement > Components: Client, java, metrics > Affects Versions: 3.0.0 > Reporter: Ronald Macmaster > Labels: newbie > Fix For: 3.0.0 > > Attachments: > 0001-HBASE-18409-MetricsConnection-client-metrics-migration.patch, > 0002-HBASE-18409-MetricsConnection-client-metrics-migrati.patch > > Original Estimate: 168h > Remaining Estimate: 168h > > Currently, the metrics for hbase-client are tailored for reporting via a > client-side JMX server. > The MetricsConnection handles the metrics management and reporting via the > metrics platform from codahale. > This approach worked well for hbase-1.3.1 when the metrics platform was still > relatively young, but it could be improved by using the new > hbase-metrics-api. > Now that we have an actual hbase-metrics-api that master, regionserver, > zookeeper, and other daemons use, it would be good to also allow the client > to leverage the metrics-api. > Then, the client could also report its metrics via Hadoop's metrics2 if > desired or through another platform that utilizes the hbase-metrics-api. > If left alone, client metrics will continue to be only barely visible through > a client-side JMX server. > The migration to the new metrics-api could be done by simply changing the > Metrics data types from codahale types to hbase-metrics types without > changing the metrics signatures of MetricsConnection unless completely > necessary. > The codahale MetricsRegistry would also have to be exchanged for a > hbase-metrics MetricsRegistry. > I found this to be a necessary change after attempting to implement my own > Reporter to use within the MetricsConnection class. > I was attempting to create a HadoopMetrics2Reporter that extends the codahale > ScheduledReporter and reports the MetricsConnection metrics to Hadoop's > metrics2 system. > The already existing infrastructure in the hbase-metrics and > hbase-metrics-api projects could be easily leveraged for a cleaner solution. > If completed successfully, users could instead access their client-side > metrics through the hbase-metrics-api. -- This message was sent by Atlassian JIRA (v6.4.14#64029)