virajjasani commented on a change in pull request #1552:
URL: https://github.com/apache/hbase/pull/1552#discussion_r412880431
##########
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:
wrapper argument can be removed?
##########
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;
+
[email protected]
+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<MetricsStoreSource> storeSources =
+ Collections.newSetFromMap(new ConcurrentHashMap<MetricsStoreSource,
Boolean>());
Review comment:
nit: `Collections.newSetFromMap(new ConcurrentHashMap<>())`
##########
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;
+
[email protected]
+public class MetricsStore {
+ private final MetricsStoreSource source;
+ private MetricsStoreWrapper storeWrapper;
+
+ public MetricsStore(final MetricsStoreWrapper wrapper, Configuration conf) {
Review comment:
We can remove conf
##########
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;
+
[email protected]
+public class MetricsStoreWrapperImpl implements MetricsStoreWrapper, Closeable
{
+
+ private final HStore store;
+ private static final Logger LOG =
LoggerFactory.getLogger(MetricsStoreWrapperImpl.class);
Review comment:
Would you like to use LOG somewhere? Or maybe dropped the idea and hence
it's not being used?
##########
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;
+
[email protected]
+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<MetricsStoreSource> storeSources =
+ Collections.newSetFromMap(new ConcurrentHashMap<MetricsStoreSource,
Boolean>());
+
+ 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:
Good to have LOG.error here?
##########
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;
+
[email protected]
+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:
Noted. For this change, is it good to give name `memstoreDataSize` ?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##########
@@ -2547,7 +2565,7 @@ public CacheConfig getCacheConfig() {
}
public static final long FIXED_OVERHEAD =
- ClassSize.align(ClassSize.OBJECT + (27 * ClassSize.REFERENCE) + (2 *
Bytes.SIZEOF_LONG)
+ ClassSize.align(ClassSize.OBJECT + (31 * ClassSize.REFERENCE) + (2 *
Bytes.SIZEOF_LONG)
Review comment:
This change is a must or it is an improvement? Just trying to understand
it better :)
##########
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:
This should be an else statement?
##########
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;
+
[email protected]
+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:
executor and runnable both could be local variable
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
##########
@@ -608,7 +613,17 @@ public boolean next(List<Cell> outResult, ScannerContext
scannerContext) throws
if (f != null) {
cell = f.transformCell(cell);
}
-
+ // tracking gets only and currently per row
+ // and not per cell. Even scans metrics at the region level are
+ // being tracked row wise.
+ if (get && !trackGets) {
+ if (!heap.current.isFileScanner()) {
+ updateMetricsStore(true);
+ } else {
+ updateMetricsStore(false);
Review comment:
Can be simplified to `updateMetricsStore(!heap.current.isFileScanner())`
----------------------------------------------------------------
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]
