[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r441335976
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionSourceImpl.java
##
@@ -302,6 +308,24 @@ void snapshot(MetricsRecordBuilder mrb, boolean ignored) {
regionNamePrefix + MetricsRegionSource.MAX_FLUSH_QUEUE_SIZE,
MetricsRegionSource.MAX_FLUSH_QUEUE_DESC),
this.regionWrapper.getMaxFlushQueueSize());
+ addCounter(mrb, this.regionWrapper.getMemstoreOnlyRowReadsCount(),
+MetricsRegionSource.ROW_READS_ONLY_ON_MEMSTORE,
+MetricsRegionSource.ROW_READS_ONLY_ON_MEMSTORE_DESC);
+ addCounter(mrb, this.regionWrapper.getMixedRowReadsCount(),
+MetricsRegionSource.MIXED_ROW_READS,
+MetricsRegionSource.MIXED_ROW_READS_ON_STORE_DESC);
Review comment:
> We don't have it already with the general read count?
This is a read count across all stores. But now what we get additionally is
per store how much is the read count that hit both memstore and files - also
one more where we say how many rows per store came out of memstore only.
Ideally the some of thees values per store should be equal to the total read
count per region.
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionSourceImpl.java
##
@@ -302,6 +308,24 @@ void snapshot(MetricsRecordBuilder mrb, boolean ignored) {
regionNamePrefix + MetricsRegionSource.MAX_FLUSH_QUEUE_SIZE,
MetricsRegionSource.MAX_FLUSH_QUEUE_DESC),
this.regionWrapper.getMaxFlushQueueSize());
+ addCounter(mrb, this.regionWrapper.getMemstoreOnlyRowReadsCount(),
+MetricsRegionSource.ROW_READS_ONLY_ON_MEMSTORE,
+MetricsRegionSource.ROW_READS_ONLY_ON_MEMSTORE_DESC);
+ addCounter(mrb, this.regionWrapper.getMixedRowReadsCount(),
+MetricsRegionSource.MIXED_ROW_READS,
+MetricsRegionSource.MIXED_ROW_READS_ON_STORE_DESC);
Review comment:
> We don't have it already with the general read count?
This is a read count across all stores. But now what we get additionally is
per store how much is the read count that hit both memstore and files - also
one more where we say how many rows per store came out of memstore only.
Ideally the some of thees values per store should be equal to the total read
count per region.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r441335976
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionSourceImpl.java
##
@@ -302,6 +308,24 @@ void snapshot(MetricsRecordBuilder mrb, boolean ignored) {
regionNamePrefix + MetricsRegionSource.MAX_FLUSH_QUEUE_SIZE,
MetricsRegionSource.MAX_FLUSH_QUEUE_DESC),
this.regionWrapper.getMaxFlushQueueSize());
+ addCounter(mrb, this.regionWrapper.getMemstoreOnlyRowReadsCount(),
+MetricsRegionSource.ROW_READS_ONLY_ON_MEMSTORE,
+MetricsRegionSource.ROW_READS_ONLY_ON_MEMSTORE_DESC);
+ addCounter(mrb, this.regionWrapper.getMixedRowReadsCount(),
+MetricsRegionSource.MIXED_ROW_READS,
+MetricsRegionSource.MIXED_ROW_READS_ON_STORE_DESC);
Review comment:
> We don't have it already with the general read count?
This is a read count across all stores. But now what we get additionally is
per store how much is the read count that hit both memstore and files - also
one more where we say how many rows per store came out of memstore only.
Ideally the sum of these values per store should be equal to the total read
count per region.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r441333721
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionSourceImpl.java
##
@@ -302,6 +308,24 @@ void snapshot(MetricsRecordBuilder mrb, boolean ignored) {
regionNamePrefix + MetricsRegionSource.MAX_FLUSH_QUEUE_SIZE,
MetricsRegionSource.MAX_FLUSH_QUEUE_DESC),
this.regionWrapper.getMaxFlushQueueSize());
+ addCounter(mrb, this.regionWrapper.getMemstoreOnlyRowReadsCount(),
+MetricsRegionSource.ROW_READS_ONLY_ON_MEMSTORE,
+MetricsRegionSource.ROW_READS_ONLY_ON_MEMSTORE_DESC);
+ addCounter(mrb, this.regionWrapper.getMixedRowReadsCount(),
+MetricsRegionSource.MIXED_ROW_READS,
+MetricsRegionSource.MIXED_ROW_READS_ON_STORE_DESC);
Review comment:
Are asking in terms of CPU that we add on while collecting the metric?
Ifyou see we do collect the metric at the HStore level per row when the
StoreScanner completes a row process. That is now a longadder. Seems it is more
performant than AtomicLong. Also the above change that we have done at the
region level is nothing but just get that metric when that runnable thread
keeps running. We don do any metric collection at this level. Are you still
thinking it may be a problem. @saintstack ? BTW thanks for your review here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r439581163
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsTableSourceImpl.java
##
@@ -311,6 +322,27 @@ void snapshot(MetricsRecordBuilder mrb, boolean ignored) {
mrb.addGauge(Interns.info(tableNamePrefix +
MetricsRegionServerSource.NUM_REFERENCE_FILES,
MetricsRegionServerSource.NUM_REFERENCE_FILES_DESC),
tableWrapperAgg.getNumReferenceFiles(tableName.getNameAsString()));
+addGauge(mrb,
tableWrapperAgg.getMemstoreReadRequestsCount(tableName.getNameAsString()),
+ MetricsRegionSource.READ_REQUEST_ON_MEMSTORE,
+ MetricsRegionSource.READ_REQUEST_ON_MEMSTORE_DESC);
+addGauge(mrb,
tableWrapperAgg.getMixedRequestsCount(tableName.getNameAsString()),
+ MetricsRegionSource.MIXED_READ_REQUEST_ON_STORE,
+ MetricsRegionSource.MIXED_READ_REQUEST_ON_STORE_DESC);
+ }
+}
+ }
+
+ private void addGauge(MetricsRecordBuilder mrb, Map metricMap,
String metricName,
+ String metricDesc) {
+if (metricMap != null) {
+ Iterator> iterator = metricMap.entrySet().iterator();
+ while (iterator.hasNext()) {
+Entry entry = iterator.next();
+// append 'store' and its name to the metric
+mrb.addGauge(Interns.info(this.tableNamePrefixPart1 + _STORE
++ entry.getKey().split(MetricsTableWrapperAggregate.UNDERSCORE)[1]
Review comment:
You mean the key should be Columnfamily? So _STORE - that should be
_ColumnFamily?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r439580608
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsTableWrapperAggregateImpl.java
##
@@ -41,39 +40,42 @@
private final HRegionServer regionServer;
private ScheduledExecutorService executor;
private Runnable runnable;
- private long period;
+ private static final int PERIOD = 45;
private ScheduledFuture tableMetricsUpdateTask;
private ConcurrentHashMap metricsTableMap
= new ConcurrentHashMap<>();
public MetricsTableWrapperAggregateImpl(final HRegionServer regionServer) {
this.regionServer = regionServer;
-this.period =
regionServer.getConfiguration().getLong(HConstants.REGIONSERVER_METRICS_PERIOD,
- HConstants.DEFAULT_REGIONSERVER_METRICS_PERIOD) + 1000;
this.executor =
CompatibilitySingletonFactory.getInstance(MetricsExecutor.class).getExecutor();
this.runnable = new TableMetricsWrapperRunnable();
-this.tableMetricsUpdateTask =
this.executor.scheduleWithFixedDelay(this.runnable, period,
- this.period, TimeUnit.MILLISECONDS);
+this.tableMetricsUpdateTask =
this.executor.scheduleWithFixedDelay(this.runnable, PERIOD,
Review comment:
This is not a bug fix and also the config based update I believe it was
just added because the MetricsREgionServer based aggregate therad was having
that config. But if you see the MetricRegionSource it is same 45 sec. The
period of updation was rather too frequent. I just changed it to be in sync
with MetricREgionSource.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r434598232
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStoreAggregateSource.java
##
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import org.apache.hadoop.hbase.metrics.BaseSource;
+import org.apache.yetus.audience.InterfaceAudience;
+
+
+@InterfaceAudience.Private
+public interface MetricsStoreAggregateSource extends BaseSource {
+ /**
+ * The name of the metrics
+ */
+ String METRICS_NAME = "Stores";
+
+ /**
+ * The name of the metrics context that metrics will be under.
+ */
+ String METRICS_CONTEXT = "regionserver";
+
+ /**
+ * Description
+ */
+ String METRICS_DESCRIPTION = "Metrics about Stores under a region";
+
+ /**
+ * The name of the metrics context that metrics will be under in jmx
+ */
+ String METRICS_JMX_CONTEXT = "RegionServer,sub=" + METRICS_NAME;
Review comment:
I think to avoid this problem of having another metric exposed at Store
level which will list all the region names and its corresponding read metrics
and also we have a region level metric where we expose across all stores in
that region how many reads happened.
Both come under different Mbean - one is sub=Regions and another is
sub=Stores. (this is added newly in this patch)
The patch already has an aggregated metric at the Table level where we
report a metric
>
NameSpace_table_store_metric
We will just use this and add a similar metric at the region level which
aggregates it per store
as we have for table level. Will that be ok ?
@saintstack - what you think?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r434376478
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStoreAggregateSource.java
##
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import org.apache.hadoop.hbase.metrics.BaseSource;
+import org.apache.yetus.audience.InterfaceAudience;
+
+
+@InterfaceAudience.Private
+public interface MetricsStoreAggregateSource extends BaseSource {
+ /**
+ * The name of the metrics
+ */
+ String METRICS_NAME = "Stores";
+
+ /**
+ * The name of the metrics context that metrics will be under.
+ */
+ String METRICS_CONTEXT = "regionserver";
+
+ /**
+ * Description
+ */
+ String METRICS_DESCRIPTION = "Metrics about Stores under a region";
+
+ /**
+ * The name of the metrics context that metrics will be under in jmx
+ */
+ String METRICS_JMX_CONTEXT = "RegionServer,sub=" + METRICS_NAME;
Review comment:
And seeing ur other comment - this is where yo uwill get per region
under each store I have how many requests. If I want to remove this then I may
have to do something like how it has been done for Table vs Store - we will
have region vs store (and it will be at the region level only).
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r434369134
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStoreAggregateSource.java
##
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import org.apache.hadoop.hbase.metrics.BaseSource;
+import org.apache.yetus.audience.InterfaceAudience;
+
+
+@InterfaceAudience.Private
+public interface MetricsStoreAggregateSource extends BaseSource {
+ /**
+ * The name of the metrics
+ */
+ String METRICS_NAME = "Stores";
+
+ /**
+ * The name of the metrics context that metrics will be under.
+ */
+ String METRICS_CONTEXT = "regionserver";
+
+ /**
+ * Description
+ */
+ String METRICS_DESCRIPTION = "Metrics about Stores under a region";
+
+ /**
+ * The name of the metrics context that metrics will be under in jmx
+ */
+ String METRICS_JMX_CONTEXT = "RegionServer,sub=" + METRICS_NAME;
Review comment:
I was just trying to push all the store level metric per region under
this. that is why collected it under RegionServer.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r434304891
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStoreAggregateSource.java
##
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import org.apache.hadoop.hbase.metrics.BaseSource;
+import org.apache.yetus.audience.InterfaceAudience;
+
+
+@InterfaceAudience.Private
+public interface MetricsStoreAggregateSource extends BaseSource {
+ /**
+ * The name of the metrics
+ */
+ String METRICS_NAME = "Stores";
Review comment:
Ok .will make it Store.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552: URL: https://github.com/apache/hbase/pull/1552#discussion_r434304818 ## File path: hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapper.java ## @@ -170,4 +170,15 @@ * all compacted store files that belong to this region */ long getMaxCompactedStoreFileRefCount(); + + /** + * @return the number of reads on memstore + */ + long getMemstoreReadRequestsCount(); Review comment: I just followed a model where currently we have accouting at table level and region level. So following the same path - We added per region per store metric and this one is nothing but a region level aggregation. This will tell on the region how many reads are from memstore. (across all stores). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552: URL: https://github.com/apache/hbase/pull/1552#discussion_r434304384 ## File path: hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionSource.java ## @@ -53,6 +53,10 @@ String COPROCESSOR_EXECUTION_STATISTICS_DESC = "Statistics for coprocessor execution times"; String REPLICA_ID = "replicaid"; String REPLICA_ID_DESC = "The replica ID of a region. 0 is primary, otherwise is secondary"; + String READ_REQUEST_ON_MEMSTORE = "readRequestCountOnMemstore"; + String READ_REQUEST_ON_MEMSTORE_DESC = "Reads happening out of memstore"; + String MIXED_READ_REQUEST_ON_STORE = "mixedReadRequestCountOnStore"; + String MIXED_READ_REQUEST_ON_STORE_DESC = "Reads happening out of files and memstore on store"; Review comment: That constants I did not clean up. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552: URL: https://github.com/apache/hbase/pull/1552#discussion_r434304341 ## File path: hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionSource.java ## @@ -53,6 +53,10 @@ String COPROCESSOR_EXECUTION_STATISTICS_DESC = "Statistics for coprocessor execution times"; String REPLICA_ID = "replicaid"; String REPLICA_ID_DESC = "The replica ID of a region. 0 is primary, otherwise is secondary"; + String READ_REQUEST_ON_MEMSTORE = "readRequestCountOnMemstore"; + String READ_REQUEST_ON_MEMSTORE_DESC = "Reads happening out of memstore"; + String MIXED_READ_REQUEST_ON_STORE = "mixedReadRequestCountOnStore"; + String MIXED_READ_REQUEST_ON_STORE_DESC = "Reads happening out of files and memstore on store"; Review comment: The one above is not needed - MEMSTORE_GET_KEY and FILE_GET_KEY in MetricsREgionServerSource because I had removed all those additional metric. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r430396089
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##
@@ -2884,4 +2901,41 @@ public int getMaxCompactedStoreFileRefCount() {
? maxCompactedStoreFileRefCount.getAsInt() : 0;
}
+ @Override
+ public long getReadRequestsFromStoreCount() {
+return getRequestsFromStore.sum();
+ }
+
+ @Override
+ public long getGetRequestsCountFromMemstore() {
+return getRequestsFromMemstore.sum();
+ }
+
+ @Override
+ public long getGetRequestsCountFromFile() {
+return getRequestsFromFile.sum();
+ }
+
+ void incrGetRequestsFromStore() {
+getRequestsFromStore.increment();
Review comment:
The one direclty inder HStore is used by the Region level and Table
level aggregators which deals with HStore. This gets printed periodically. The
other one is at the MetricsStore level which is the real time one. For every
request it will be displayed at JMX MBean level.
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsTableWrapperAggregateImpl.java
##
@@ -70,25 +69,36 @@ public void run() {
localMetricsTableMap.put(tbl, mt);
}
if (r.getStores() != null) {
+ long memstoreReadCount = 0l;
+ long fileReadCount = 0l;
+ String familyName = null;
for (Store store : r.getStores()) {
+familyName = store.getColumnFamilyName();
+
mt.storeFileCount += store.getStorefilesCount();
-mt.memstoreSize += (store.getMemStoreSize().getDataSize() +
- store.getMemStoreSize().getHeapSize() +
store.getMemStoreSize().getOffHeapSize());
+mt.memstoreSize += (store.getMemStoreSize().getDataSize()
++ store.getMemStoreSize().getHeapSize() +
store.getMemStoreSize().getOffHeapSize());
mt.storeFileSize += store.getStorefilesSize();
mt.referenceFileCount += store.getNumReferenceFiles();
-mt.maxStoreFileAge = Math.max(mt.maxStoreFileAge,
store.getMaxStoreFileAge().getAsLong());
-mt.minStoreFileAge = Math.min(mt.minStoreFileAge,
store.getMinStoreFileAge().getAsLong());
-mt.totalStoreFileAge =
(long)store.getAvgStoreFileAge().getAsDouble() *
-store.getStorefilesCount();
+mt.maxStoreFileAge =
+Math.max(mt.maxStoreFileAge,
store.getMaxStoreFileAge().getAsLong());
+mt.minStoreFileAge =
+Math.min(mt.minStoreFileAge,
store.getMinStoreFileAge().getAsLong());
+mt.totalStoreFileAge =
+(long) store.getAvgStoreFileAge().getAsDouble() *
store.getStorefilesCount();
mt.storeCount += 1;
+memstoreReadCount += store.getGetRequestsCountFromMemstore();
+fileReadCount += store.getGetRequestsCountFromFile();
+mt.storeMemstoreGetCount.putIfAbsent(familyName,
memstoreReadCount);
+mt.storeFileGetCount.putIfAbsent(familyName, fileReadCount);
}
+
mt.regionCount += 1;
mt.readRequestCount += r.getReadRequestsCount();
- mt.filteredReadRequestCount +=
getFilteredReadRequestCount(tbl.getNameAsString());
+ mt.filteredReadRequestCount += r.getFilteredReadRequestsCount();
Review comment:
This was wrong. It is a simple change. So I thought it is better to make
this change hhere. If you are particular i can make the change in separete JIRA.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r428789194
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##
@@ -1002,6 +1012,14 @@ public Void call() throws IOException {
} finally {
this.lock.writeLock().unlock();
this.archiveLock.unlock();
+ // moving it after the unlocking so
+ // that metrics closure does not affect them
+ if (this.metricsStore != null) {
+metricsStore.close();
Review comment:
Yes. It will do the deregister.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r428788807
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStoreSourceImpl.java
##
@@ -0,0 +1,211 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.hadoop.hbase.metrics.Counter;
+import org.apache.hadoop.hbase.metrics.Interns;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class MetricsStoreSourceImpl implements MetricsStoreSource {
+
+ private MetricsStoreWrapper storeWrapper;
+ private MetricsStoreAggregateSourceImpl aggreagate;
+ private AtomicBoolean closed = new AtomicBoolean(false);
+
+ private String storeNamePrefix;
+ private final MetricRegistry registry;
+ private static final Logger LOG =
LoggerFactory.getLogger(MetricsStoreSourceImpl.class);
+ String storeReadsKey;
+
+ String memstoreReadsKey;
+ String fileReadsKey;
+ private final Counter storeReads;
+ private final Counter memstoreReads;
+ private final Counter fileReads;
+
+ public MetricsStoreSourceImpl(MetricsStoreWrapper storeWrapper,
+ MetricsStoreAggregateSourceImpl aggreagate) {
+this.storeWrapper = storeWrapper;
+this.aggreagate = aggreagate;
+aggreagate.register(this);
+
+LOG.debug("Creating new MetricsRegionSourceImpl for table " +
storeWrapper.getStoreName() + " "
++ storeWrapper.getRegionName());
+
+// we are using the hbase-metrics API
+registry = aggreagate.getMetricRegistry();
+
+storeNamePrefix = "Namespace_" + storeWrapper.getNamespace() + "_table_"
++ storeWrapper.getTableName() + "_region_" +
storeWrapper.getRegionName() + "_store_"
++ storeWrapper.getStoreName() + "_metric_";
+
+String suffix = "Count";
+
+storeReadsKey = storeNamePrefix + MetricsRegionServerSource.GET_KEY +
suffix;
+// all the counters are hbase-metrics API
+storeReads = registry.counter(storeReadsKey);
+
+memstoreReadsKey = storeNamePrefix +
MetricsRegionServerSource.MEMSTORE_GET_KEY + suffix;
+memstoreReads = registry.counter(memstoreReadsKey);
+
+fileReadsKey = storeNamePrefix + MetricsRegionServerSource.FILE_GET_KEY +
suffix;
+fileReads = registry.counter(fileReadsKey);
+
+ }
+
+ @Override
+ public void close() {
+boolean wasClosed = closed.getAndSet(true);
+
+// Has someone else already closed this for us?
+if (wasClosed) {
+ return;
+}
+
+// Before removing the metrics remove this region from the aggregate
region bean.
+// This should mean that it's unlikely that snapshot and close happen at
the same time.
+aggreagate.deregister(this);
+
+// While it's un-likely that snapshot and close happen at the same time
it's still possible.
+// So grab the lock to ensure that all calls to snapshot are done before
we remove the metrics
+synchronized (this) {
+ if (LOG.isTraceEnabled()) {
+LOG.trace("Removing store Metrics: " + storeWrapper.getStoreName());
+ }
+
+ registry.remove(storeReadsKey);
+ registry.remove(memstoreReadsKey);
+ registry.remove(fileReadsKey);
+
+ storeWrapper = null;
+}
+ }
+
+ @Override
+ public int compareTo(MetricsStoreSource source) {
+if (!(source instanceof MetricsStoreSourceImpl)) {
+ return -1;
+}
+
+MetricsStoreSourceImpl impl = (MetricsStoreSourceImpl) source;
+if (impl == null) {
+ return -1;
+}
+
+// TODO : make this better
+return Long.compare(this.storeWrapper.getStoreName().hashCode(),
+ impl.storeWrapper.getStoreName().hashCode());
+ }
+
+ @Override
+ public void updateGet() {
+storeReads.increment();
+ }
+
+ @Override
+ public void updateMemtoreGet() {
+memstoreReads.increment();
+ }
+
+ @Override
+ public void updateFileGet() {
+fileReads.increment();
+ }
+
+ @Override
+ public boolean
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r428788326
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionSourceImpl.java
##
@@ -302,6 +302,14 @@ void snapshot(MetricsRecordBuilder mrb, boolean ignored) {
regionNamePrefix + MetricsRegionSource.MAX_FLUSH_QUEUE_SIZE,
MetricsRegionSource.MAX_FLUSH_QUEUE_DESC),
this.regionWrapper.getMaxFlushQueueSize());
+ mrb.addCounter(
Review comment:
At the store level only it comes as per region per store. This is
something we already have. Jut adding those two new metric here. So should be
ok.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552: URL: https://github.com/apache/hbase/pull/1552#discussion_r428787492 ## File path: hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSource.java ## @@ -402,6 +402,8 @@ String DELETE_BATCH_KEY = "deleteBatch"; String GET_SIZE_KEY = "getSize"; String GET_KEY = "get"; + String MEMSTORE_GET_KEY = "getsOnMemstore"; + String FILE_GET_KEY = "getsOnFile"; Review comment: Some of the gets that lands on the StoreScanners does not actually get accounted into actual get. Probably that row does not exist. So I thought it is better to track both. Also the overhead is very minimal. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r424941958
##
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStoreAggregateSource.java
##
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import org.apache.hadoop.hbase.metrics.BaseSource;
+import org.apache.yetus.audience.InterfaceAudience;
+
+
+@InterfaceAudience.Private
+public interface MetricsStoreAggregateSource extends BaseSource {
+ /**
+ * The name of the metrics
+ */
+ String METRICS_NAME = "Stores";
+
+ /**
+ * The name of the metrics context that metrics will be under.
+ */
+ String METRICS_CONTEXT = "regionserver";
+
+ /**
+ * Description
+ */
+ String METRICS_DESCRIPTION = "Metrics about Stores under a region";
Review comment:
I have now tried to consolidate the metric at the table level also. Pls
have a look. @anoopsjohn .
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r414469707
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##
@@ -2898,4 +2915,40 @@ public int getMaxCompactedStoreFileRefCount() {
? maxCompactedStoreFileRefCount.getAsInt() : 0;
}
+ @Override
+ public long getReadRequestsFromStoreCount() {
+return getRequestsFromStore.sum();
+ }
+
+ @Override
+ public long getGetRequestsCountFromMemstore() {
+return getRequestsFromMemstore.sum();
+ }
+
+ @Override
+ public long getGetRequestsCountFromFile() {
+return getRequestsFromFile.sum();
+ }
+
+ void incrGetRequestsFromStore() {
+getRequestsFromStore.increment();
+if (metricsStore != null) {
+ metricsStore.updateGet();
+}
+ }
+
+ void updateMetricsStore(boolean memstoreRead) {
+if (memstoreRead) {
+ getRequestsFromMemstore.increment();
+} else {
+ getRequestsFromFile.increment();
+}
+if (metricsStore != null) {
+ if (memstoreRead) {
+metricsStore.updateMemstoreGet();
+ }
+ metricsStore.updateFileGet();
Review comment:
good catch. Thanks for the reviews.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r414466487
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStoreWrapperImpl.java
##
@@ -0,0 +1,194 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.OptionalDouble;
+import java.util.OptionalLong;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hadoop.hbase.CompatibilitySingletonFactory;
+import org.apache.hadoop.metrics2.MetricsExecutor;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class MetricsStoreWrapperImpl implements MetricsStoreWrapper, Closeable
{
+
+ private final HStore store;
+ private static final Logger LOG =
LoggerFactory.getLogger(MetricsStoreWrapperImpl.class);
+
+ public static final int PERIOD = 45;
+ public static final String UNKNOWN = "unknown";
+ private ScheduledExecutorService executor;
+ private Runnable runnable;
Review comment:
Sure.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r414464537
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStoreWrapperImpl.java
##
@@ -0,0 +1,194 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.OptionalDouble;
+import java.util.OptionalLong;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hadoop.hbase.CompatibilitySingletonFactory;
+import org.apache.hadoop.metrics2.MetricsExecutor;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class MetricsStoreWrapperImpl implements MetricsStoreWrapper, Closeable
{
+
+ private final HStore store;
+ private static final Logger LOG =
LoggerFactory.getLogger(MetricsStoreWrapperImpl.class);
+
+ public static final int PERIOD = 45;
+ public static final String UNKNOWN = "unknown";
+ private ScheduledExecutorService executor;
+ private Runnable runnable;
+ // add others also. check if anything is redundant
+ private long numStoreFiles;
+ private long memstoreSize;
+ private long storeFileSize;
+ private long getsFromMemstore;
+ private long getsOnStore;
+ private long getsOnFile;
+ private long numReferenceFiles;
+ private long minStoreFileAge;
+ private long maxStoreFileAge;
+ private long avgStoreFileAge;
+ private long numHFiles;
+ private int storeRefCount;
+
+ private ScheduledFuture storeMetricUpdateTask;
+
+ public MetricsStoreWrapperImpl(HStore store) {
+this.store = store;
+this.executor =
CompatibilitySingletonFactory.getInstance(MetricsExecutor.class).getExecutor();
+this.runnable = new HStoreMetricsWrapperRunnable();
+this.storeMetricUpdateTask =
+this.executor.scheduleWithFixedDelay(this.runnable, PERIOD, PERIOD,
TimeUnit.SECONDS);
+ }
+
+ @Override
+ public void close() throws IOException {
+storeMetricUpdateTask.cancel(true);
+ }
+
+ @Override
+ public String getStoreName() {
+return store.getColumnFamilyName();
+ }
+
+ @Override
+ public String getRegionName() {
+return store.getRegionInfo().getRegionNameAsString();
+ }
+
+ @Override
+ public String getTableName() {
+return store.getRegionInfo().getTable().getNameAsString();
+ }
+
+ @Override
+ public String getNamespace() {
+return store.getTableName().getNamespaceAsString();
+ }
+
+ @Override
+ public long getNumStoreFiles() {
+return numStoreFiles;
+ }
+
+ @Override
+ public long getMemStoreSize() {
+// todo : change this - we need to expose data, heapsize and
offheapdatasize
Review comment:
For now lets be it this way. All other metrics has to be changed. We can
do it that time.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r414464389
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStoreWrapperImpl.java
##
@@ -0,0 +1,194 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.OptionalDouble;
+import java.util.OptionalLong;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hadoop.hbase.CompatibilitySingletonFactory;
+import org.apache.hadoop.metrics2.MetricsExecutor;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class MetricsStoreWrapperImpl implements MetricsStoreWrapper, Closeable
{
+
+ private final HStore store;
+ private static final Logger LOG =
LoggerFactory.getLogger(MetricsStoreWrapperImpl.class);
Review comment:
Removed it.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r414463943
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStore.java
##
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.CompatibilitySingletonFactory;
+import org.apache.yetus.audience.InterfaceAudience;
+
+@InterfaceAudience.Private
+public class MetricsStore {
+ private final MetricsStoreSource source;
+ private MetricsStoreWrapper storeWrapper;
+
+ public MetricsStore(final MetricsStoreWrapper wrapper, Configuration conf) {
Review comment:
Done.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r414463271
##
File path:
hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStoreAggregateSourceImpl.java
##
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hadoop.hbase.metrics.BaseSourceImpl;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.impl.JmxCacheBuster;
+import org.apache.hadoop.metrics2.lib.MetricsExecutorImpl;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class MetricsStoreAggregateSourceImpl extends BaseSourceImpl
+implements MetricsStoreAggregateSource {
+ private static final Logger LOG =
LoggerFactory.getLogger(MetricsStoreAggregateSourceImpl.class);
+
+ private final MetricsExecutorImpl executor = new MetricsExecutorImpl();
+
+ private final Set storeSources =
+ Collections.newSetFromMap(new ConcurrentHashMap());
+
+ public MetricsStoreAggregateSourceImpl() {
+this(METRICS_NAME, METRICS_DESCRIPTION, METRICS_CONTEXT,
METRICS_JMX_CONTEXT);
+ }
+
+
+ public MetricsStoreAggregateSourceImpl(String metricsName,
+ String metricsDescription,
+ String metricsContext,
+ String metricsJmxContext) {
+super(metricsName, metricsDescription, metricsContext, metricsJmxContext);
+
+// Every few mins clean the JMX cache.
+executor.getExecutor().scheduleWithFixedDelay(new Runnable() {
+ public void run() {
+JmxCacheBuster.clearJmxCache();
+ }
+}, 5, 5, TimeUnit.MINUTES);
+ }
+
+ public MetricRegistry getMetricRegistry() {
+return registry;
+ }
+
+ @Override
+ public void register(MetricsStoreSource source) {
+storeSources.add(source);
+clearCache();
+ }
+
+ @Override
+ public void deregister(MetricsStoreSource toRemove) {
+try {
+ storeSources.remove(toRemove);
+} catch (Exception e) {
+ // Ignored. If this errors out it means that someone is double
+ // closing the region source and the region is already nulled out.
+ LOG.info(
Review comment:
Actually the removal is not needed to handle throw catch. I just used
what was being done in MetricsREgionAggregateSource. Seems that is not needed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r414461189
##
File path:
hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsStoreAggregateSourceImpl.java
##
@@ -0,0 +1,116 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.regionserver;
+
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.hadoop.hbase.metrics.BaseSourceImpl;
+import org.apache.hadoop.hbase.metrics.MetricRegistry;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.impl.JmxCacheBuster;
+import org.apache.hadoop.metrics2.lib.MetricsExecutorImpl;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@InterfaceAudience.Private
+public class MetricsStoreAggregateSourceImpl extends BaseSourceImpl
+implements MetricsStoreAggregateSource {
+ private static final Logger LOG =
LoggerFactory.getLogger(MetricsStoreAggregateSourceImpl.class);
+
+ private final MetricsExecutorImpl executor = new MetricsExecutorImpl();
+
+ private final Set storeSources =
+ Collections.newSetFromMap(new ConcurrentHashMap());
Review comment:
the compiler was saying some warnings. Hence left it as is.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] ramkrish86 commented on a change in pull request #1552: HBASE-24205 Create metric to know the number of reads that happens fr…
ramkrish86 commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r414459588
##
File path:
hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionServerSourceFactoryImpl.java
##
@@ -45,6 +46,16 @@ private synchronized MetricsRegionAggregateSourceImpl
getRegionAggregate() {
}
}
+ private synchronized MetricsStoreAggregateSourceImpl
+ getStoreAggregate(MetricsStoreWrapper wrapper) {
Review comment:
Ya not needed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
