[jira] [Comment Edited] (HBASE-15518) Add Per-Table metrics back

["Enis Soztutar (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HBASE-15518?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15225559#comment-15225559 ] 

Enis Soztutar edited comment on HBASE-15518 at 4/5/16 3:03 AM:
---------------------------------------------------------------

Thanks [~aliciashu] for the patch. Couple of comments: 
 - Please check the above hadoopqa warnings, and address those. 
 - Having both {{num}} and {{Count}} in the metric name does not make sense. Lets use {{readRequestCount}} / {{writeRequestCount}} to be consistent with RS-level metric names. Same thing for total request count. 
{code}+  String NUM_READ_REQUESTS_COUNT = "numReadRequestsCount";{code}
 - Move this class inside the MetricsTableWrapperAggregateImpl. No need to be top level. It should be declared private. 
{code}
+public class MetricsTableValues {
{code} 
 - Instead of using the table aggregate wrapper directly in the regionserver, lets do a wrapper like the MetricsTable to hold the aggregate source and aggregate wrapper. We can instantiate that in the regionserver level. 
{code}
+  MetricsTableWrapperAggregate metricsTableWrapperAgg;
{code}
- You don't need this ConcurrentHashMap for metricsTableMap. It should be a locally allocated map inside the method that uses it. 
{code}
+  private ConcurrentHashMap<TableName, MetricsTableValues> metricsTableMap = new ConcurrentHashMap<>();
{code}
- otherwise looks good. 
 - Let me test this patch with my ycsb setup. 


was (Author: enis):
Thanks [~aliciashu] for the patch. Couple of comments: 
 - Please check the above hadoopqa warnings, and address those. 
 - Having both {{num}} and {{Count}} in the metric name does not make sense. Lets use {{readRequestCount}} / {{writeRequestCount}} to be consistent with RS-level metric names. Same thing for total request count. 
{code}+  String NUM_READ_REQUESTS_COUNT = "numReadRequestsCount";
 - Move this class inside the MetricsTableWrapperAggregateImpl. No need to be top level. It should be declared private. 
{code}
+public class MetricsTableValues {
{code} 
 - Instead of using the table aggregate wrapper directly in the regionserver, lets do a wrapper like the MetricsTable to hold the aggregate source and aggregate wrapper. We can instantiate that in the regionserver level. 
{code}
+  MetricsTableWrapperAggregate metricsTableWrapperAgg;
{code}
- You don't need this ConcurrentHashMap for metricsTableMap. It should be a locally allocated map inside the method that uses it. 
{code}
+  private ConcurrentHashMap<TableName, MetricsTableValues> metricsTableMap = new ConcurrentHashMap<>();
{code}
- otherwise looks good. 
 - Let me test this patch with my ycsb setup. 

> Add Per-Table metrics back
> --------------------------
>
>                 Key: HBASE-15518
>                 URL: https://issues.apache.org/jira/browse/HBASE-15518
>             Project: HBase
>          Issue Type: Sub-task
>            Reporter: Enis Soztutar
>            Assignee: Alicia Ying Shu
>             Fix For: 2.0.0, 1.4.0
>
>         Attachments: HBASE-15518.patch
>
>
> We used to have per-table metrics, but it was removed in some restructuring. We have per-region metrics, and per-regionserver metrics, but nothing in between. 
> For majority of users, per-region is too granular, they are mostly interested in table level aggregates. This is especially useful in multi-tenant cases where a table's disk usage, number of requests, etc can be made much more visible. 
> In this jira, we'll add the basic infrastructure to add a single (or a few) per-table metrics. Than we can improve on that by adding remaining metrics from the region server level. 
> The plan is to NOT aggregate per-table metrics at master for now. Just aggregation of per-region metrics at the per-table level for every regionserver. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)