saintstack commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r434207957
##########
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:
In other words, it'd be better if the accounting aligned.
##########
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:
Yeah, don't we have this accounted already? Seems redundant.
##########
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;
+
+
[email protected]
+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:
This model doesn't seem right. There is no such thing as a Store on a
RegionServer. The RegionServer hosts Regions. A Region hosts Stores. This is
an aggregate of all Stores on the RegionServer?
I could imagine RegionServer,sub=Region,sub=Store... with a Bean per Store
but then we'd probably have too many MBeans.
Do we have a Region at this level in MBean hierarchy?
##########
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:
... this if for the Region? Can I ask for the number of reads on
memstore on a Store?
##########
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:
I don't follow the above. We already have account at the store level per
region? Why then would we add these counters?
##########
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;
+
+
[email protected]
+public interface MetricsStoreAggregateSource extends BaseSource {
+ /**
+ * The name of the metrics
+ */
+ String METRICS_NAME = "Stores";
Review comment:
If a single Store, should be called 'Store' not 'Stores'?
##########
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;
+
+
[email protected]
+public interface MetricsStoreAggregateSource extends BaseSource {
+ /**
+ * The name of the metrics
+ */
+ String METRICS_NAME = "Stores";
Review comment:
Metrics for a Store for more than one Store? If for more than one Store,
why not in 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:
[email protected]
