From e3217a4f8b2fea0c778b7daa027309551a2747e8 Mon Sep 17 00:00:00 2001 From: Elliott Clark Date: Wed, 29 Jul 2015 15:04:46 -0700 Subject: [PATCH] HBASE-14166 Per-Region metrics can be stale --- .../regionserver/MetricsRegionAggregateSource.java | 3 ++ .../hbase/regionserver/MetricsRegionWrapper.java | 2 + .../MetricsRegionAggregateSourceImpl.java | 46 ++++++++++++++++------ .../regionserver/MetricsRegionSourceImpl.java | 23 ++++++++--- .../hadoop/metrics2/impl/JmxCacheBuster.java | 9 ++++- .../metrics2/lib/DefaultMetricsSystemHelper.java | 10 +++++ .../metrics2/lib/DynamicMetricsRegistry.java | 16 ++++++++ .../hadoop/metrics2/lib/MetricsExecutorImpl.java | 8 ++-- .../regionserver/TestMetricsRegionSourceImpl.java | 5 +++ .../regionserver/MetricsRegionWrapperImpl.java | 5 +++ .../regionserver/MetricsRegionWrapperStub.java | 5 +++ 11 files changed, 110 insertions(+), 22 deletions(-) create mode 100644 hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/DefaultMetricsSystemHelper.java diff --git a/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionAggregateSource.java b/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionAggregateSource.java index 0ef74a9..578ce49 100644 --- a/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionAggregateSource.java +++ b/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionAggregateSource.java @@ -46,6 +46,9 @@ public interface MetricsRegionAggregateSource extends BaseSource { */ String METRICS_JMX_CONTEXT = "RegionServer,sub=" + METRICS_NAME; + String NUM_REGIONS = "numRegions"; + String NUMBER_OF_REGIONS_DESC = "Number of regions in the metrics system"; + /** * Register a MetricsRegionSource as being open. * diff --git a/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapper.java b/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapper.java index 7d61f81..cfc0b66 100644 --- a/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapper.java +++ b/hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapper.java @@ -84,6 +84,8 @@ public interface MetricsRegionWrapper { long getNumCompactionsCompleted(); + int getRegionHashCode(); + /** * Get the time spent by coprocessors in this region. */ diff --git a/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionAggregateSourceImpl.java b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionAggregateSourceImpl.java index ab7255e..4db2741 100644 --- a/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionAggregateSourceImpl.java +++ b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionAggregateSourceImpl.java @@ -18,23 +18,27 @@ package org.apache.hadoop.hbase.regionserver; -import java.util.TreeSet; +import java.util.HashSet; +import java.util.concurrent.TimeUnit; import java.util.concurrent.locks.ReentrantReadWriteLock; import org.apache.hadoop.hbase.classification.InterfaceAudience; import org.apache.hadoop.hbase.metrics.BaseSourceImpl; 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.Interns; +import org.apache.hadoop.metrics2.lib.MetricsExecutorImpl; @InterfaceAudience.Private public class MetricsRegionAggregateSourceImpl extends BaseSourceImpl implements MetricsRegionAggregateSource { - // lock to guard against concurrent access to regionSources private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); + private final MetricsExecutorImpl executor = new MetricsExecutorImpl(); - private final TreeSet regionSources = - new TreeSet(); + private final HashSet regionSources = + new HashSet(); public MetricsRegionAggregateSourceImpl() { this(METRICS_NAME, METRICS_DESCRIPTION, METRICS_CONTEXT, METRICS_JMX_CONTEXT); @@ -46,13 +50,21 @@ public class MetricsRegionAggregateSourceImpl extends BaseSourceImpl 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(true); + } + }, 5, 5, TimeUnit.MINUTES); } @Override public void register(MetricsRegionSource source) { lock.writeLock().lock(); try { - regionSources.add((MetricsRegionSourceImpl) source); + regionSources.add(source); + clearCache(); } finally { lock.writeLock().unlock(); } @@ -63,11 +75,23 @@ public class MetricsRegionAggregateSourceImpl extends BaseSourceImpl lock.writeLock().lock(); try { regionSources.remove(source); + clearCache(); } finally { lock.writeLock().unlock(); } } + private synchronized void clearCache() { + JmxCacheBuster.clearJmxCache(true); + executor.getExecutor().schedule(new Runnable() { + @Override + public void run() { + JmxCacheBuster.clearJmxCache(); + } + }, 30, TimeUnit.SECONDS); + + } + /** * Yes this is a get function that doesn't return anything. Thanks Hadoop for breaking all * expectations of java programmers. Instead of returning anything Hadoop metrics expects @@ -78,21 +102,21 @@ public class MetricsRegionAggregateSourceImpl extends BaseSourceImpl */ @Override public void getMetrics(MetricsCollector collector, boolean all) { - - MetricsRecordBuilder mrb = collector.addRecord(metricsName); if (regionSources != null) { lock.readLock().lock(); try { - for (MetricsRegionSourceImpl regionMetricSource : regionSources) { - regionMetricSource.snapshot(mrb, all); + for (MetricsRegionSource regionMetricSource : regionSources) { + if (regionMetricSource instanceof MetricsRegionSourceImpl) { + ((MetricsRegionSourceImpl) regionMetricSource).snapshot(mrb, all); + } } } finally { lock.readLock().unlock(); } + mrb.addGauge(Interns.info(NUM_REGIONS, NUMBER_OF_REGIONS_DESC), regionSources.size()); + metricsRegistry.snapshot(mrb, all); } - - metricsRegistry.snapshot(mrb, all); } } diff --git a/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionSourceImpl.java b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionSourceImpl.java index df23942..9d86405 100644 --- a/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionSourceImpl.java +++ b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionSourceImpl.java @@ -26,6 +26,8 @@ import org.apache.commons.math.stat.descriptive.DescriptiveStatistics; import org.apache.hadoop.hbase.classification.InterfaceAudience; import org.apache.hadoop.metrics2.MetricsRecordBuilder; import org.apache.hadoop.metrics2.impl.JmxCacheBuster; +import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem; +import org.apache.hadoop.metrics2.lib.DefaultMetricsSystemHelper; import org.apache.hadoop.metrics2.lib.DynamicMetricsRegistry; import org.apache.hadoop.metrics2.lib.Interns; import org.apache.hadoop.metrics2.lib.MutableCounterLong; @@ -34,13 +36,13 @@ import org.apache.hadoop.metrics2.lib.MutableHistogram; @InterfaceAudience.Private public class MetricsRegionSourceImpl implements MetricsRegionSource { - private final MetricsRegionWrapper regionWrapper; + private static final Log LOG = LogFactory.getLog(MetricsRegionSourceImpl.class); private boolean closed = false; + private MetricsRegionWrapper regionWrapper; private MetricsRegionAggregateSourceImpl agg; private DynamicMetricsRegistry registry; - private static final Log LOG = LogFactory.getLog(MetricsRegionSourceImpl.class); private String regionNamePrefix; private String regionPutKey; @@ -100,6 +102,7 @@ public class MetricsRegionSourceImpl implements MetricsRegionSource { closed = true; agg.deregister(this); + LOG.trace("Removing region Metrics: " + regionWrapper.getRegionName()); registry.removeMetric(regionPutKey); registry.removeMetric(regionDeleteKey); @@ -111,6 +114,7 @@ public class MetricsRegionSourceImpl implements MetricsRegionSource { registry.removeMetric(regionGetKey); registry.removeMetric(regionScanNextKey); + this.regionWrapper = null; JmxCacheBuster.clearJmxCache(); } @@ -162,7 +166,7 @@ public class MetricsRegionSourceImpl implements MetricsRegionSource { @Override public int hashCode() { - return this.regionWrapper.getRegionName().hashCode(); + return regionWrapper.getRegionHashCode(); } @Override @@ -186,17 +190,24 @@ public class MetricsRegionSourceImpl implements MetricsRegionSource { MetricsRegionServerSource.MEMSTORE_SIZE_DESC), this.regionWrapper.getMemstoreSize()); mrb.addGauge(Interns.info(regionNamePrefix + MetricsRegionServerSource.STOREFILE_SIZE, - MetricsRegionServerSource.STOREFILE_SIZE_DESC), + MetricsRegionServerSource.STOREFILE_SIZE_DESC), this.regionWrapper.getStoreFileSize()); mrb.addCounter(Interns.info(regionNamePrefix + MetricsRegionSource.COMPACTIONS_COMPLETED_COUNT, - MetricsRegionSource.COMPACTIONS_COMPLETED_DESC), + MetricsRegionSource.COMPACTIONS_COMPLETED_DESC), this.regionWrapper.getNumCompactionsCompleted()); mrb.addCounter(Interns.info(regionNamePrefix + MetricsRegionSource.NUM_BYTES_COMPACTED_COUNT, - MetricsRegionSource.NUM_BYTES_COMPACTED_DESC), + MetricsRegionSource.NUM_BYTES_COMPACTED_DESC), this.regionWrapper.getNumBytesCompacted()); mrb.addCounter(Interns.info(regionNamePrefix + MetricsRegionSource.NUM_FILES_COMPACTED_COUNT, MetricsRegionSource.NUM_FILES_COMPACTED_DESC), this.regionWrapper.getNumFilesCompacted()); + mrb.addCounter(Interns.info(regionNamePrefix + MetricsRegionServerSource.READ_REQUEST_COUNT, + MetricsRegionServerSource.READ_REQUEST_COUNT_DESC), + this.regionWrapper.getReadRequestCount()); + mrb.addCounter(Interns.info(regionNamePrefix + MetricsRegionServerSource.WRITE_REQUEST_COUNT, + MetricsRegionServerSource.WRITE_REQUEST_COUNT_DESC), + this.regionWrapper.getWriteRequestCount()); + for (Map.Entry entry : this.regionWrapper .getCoprocessorExecutionStatistics() .entrySet()) { diff --git a/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/impl/JmxCacheBuster.java b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/impl/JmxCacheBuster.java index 5d83150..644ba24 100644 --- a/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/impl/JmxCacheBuster.java +++ b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/impl/JmxCacheBuster.java @@ -49,11 +49,17 @@ public class JmxCacheBuster { /** * For JMX to forget about all previously exported metrics. */ + public static void clearJmxCache() { + clearJmxCache(false); + } + + public static void clearJmxCache(boolean force) { //If there are more then 100 ms before the executor will run then everything should be merged. synchronized (lock) { - if (fut == null || (!fut.isDone() && fut.getDelay(TimeUnit.MILLISECONDS) > 100)) return; + if (!force + && (fut == null || (!fut.isDone() && fut.getDelay(TimeUnit.MILLISECONDS) > 100))) return; fut = executor.getExecutor().schedule(new JmxCacheBusterRunnable(), 5, TimeUnit.SECONDS); } } @@ -71,7 +77,6 @@ public class JmxCacheBuster { DefaultMetricsSystem.instance().stop(); DefaultMetricsSystem.instance().start(); } - } catch (Exception exception ) { LOG.debug("error clearing the jmx it appears the metrics system hasn't been started", exception); } diff --git a/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/DefaultMetricsSystemHelper.java b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/DefaultMetricsSystemHelper.java new file mode 100644 index 0000000..16b7b7b --- /dev/null +++ b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/DefaultMetricsSystemHelper.java @@ -0,0 +1,10 @@ +package org.apache.hadoop.metrics2.lib; + +/** + * Created by elliott on 7/29/15. + */ +public class DefaultMetricsSystemHelper { + public static void removeObjectName(String name) { + DefaultMetricsSystem.INSTANCE.removeObjectName(name); + } +} diff --git a/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/DynamicMetricsRegistry.java b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/DynamicMetricsRegistry.java index 281200c..34e1de6 100644 --- a/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/DynamicMetricsRegistry.java +++ b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/DynamicMetricsRegistry.java @@ -21,6 +21,8 @@ package org.apache.hadoop.metrics2.lib; import java.util.Collection; import java.util.concurrent.ConcurrentMap; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.apache.hadoop.hbase.classification.InterfaceAudience; import org.apache.hadoop.metrics2.MetricsException; import org.apache.hadoop.metrics2.MetricsInfo; @@ -44,6 +46,8 @@ import com.google.common.collect.Maps; */ @InterfaceAudience.Private public class DynamicMetricsRegistry { + private static final Log LOG = LogFactory.getLog(DynamicMetricsRegistry.class); + private final ConcurrentMap metricsMap = Maps.newConcurrentMap(); private final ConcurrentMap tagsMap = @@ -405,6 +409,11 @@ public class DynamicMetricsRegistry { * @param name name of the metric to remove */ public void removeMetric(String name) { + try { + DefaultMetricsSystemHelper.removeObjectName(name); + } catch (Exception e) { + LOG.debug("Error when trying to clean the metrics cache."); + } metricsMap.remove(name); } @@ -540,6 +549,13 @@ public class DynamicMetricsRegistry { } public void clearMetrics() { + for (String name:metricsMap.keySet()) { + try { + DefaultMetricsSystemHelper.removeObjectName(name); + } catch (Exception e) { + LOG.debug("Error when trying to clean the metrics cache."); + } + } metricsMap.clear(); } } diff --git a/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/MetricsExecutorImpl.java b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/MetricsExecutorImpl.java index 18a759e..897084e 100644 --- a/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/MetricsExecutorImpl.java +++ b/hbase-hadoop2-compat/src/main/java/org/apache/hadoop/metrics2/lib/MetricsExecutorImpl.java @@ -27,7 +27,9 @@ import org.apache.hadoop.hbase.classification.InterfaceAudience; import org.apache.hadoop.metrics2.MetricsExecutor; /** - * Class to handle the ScheduledExecutorService{@link ScheduledExecutorService} used by MetricMutableQuantiles{@link MetricMutableQuantiles} + * Class to handle the ScheduledExecutorService{@link ScheduledExecutorService} used by + * MetricMutableQuantiles{@link MetricMutableQuantiles}, MetricsRegionAggregateSourceImpl, and + * JmxCacheBuster */ @InterfaceAudience.Private public class MetricsExecutorImpl implements MetricsExecutor { @@ -46,8 +48,8 @@ public class MetricsExecutorImpl implements MetricsExecutor { private enum ExecutorSingleton { INSTANCE; - - private final ScheduledExecutorService scheduler = new ScheduledThreadPoolExecutor(1, new ThreadPoolExecutorThreadFactory("HBase-Metrics2-")); + private final ScheduledExecutorService scheduler = new ScheduledThreadPoolExecutor(1, + new ThreadPoolExecutorThreadFactory("HBase-Metrics2-")); } private static class ThreadPoolExecutorThreadFactory implements ThreadFactory { diff --git a/hbase-hadoop2-compat/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsRegionSourceImpl.java b/hbase-hadoop2-compat/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsRegionSourceImpl.java index 4be8905..28b1d62 100644 --- a/hbase-hadoop2-compat/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsRegionSourceImpl.java +++ b/hbase-hadoop2-compat/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsRegionSourceImpl.java @@ -125,6 +125,11 @@ public class TestMetricsRegionSourceImpl { } @Override + public int getRegionHashCode() { + return regionName.hashCode(); + } + + @Override public Map getCoprocessorExecutionStatistics() { return null; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapperImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapperImpl.java index be05f3e..78df787 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapperImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapperImpl.java @@ -135,6 +135,11 @@ public class MetricsRegionWrapperImpl implements MetricsRegionWrapper, Closeable return this.region.compactionsFinished.get(); } + @Override + public int getRegionHashCode() { + return this.region.hashCode(); + } + public class HRegionMetricsWrapperRunnable implements Runnable { @Override diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapperStub.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapperStub.java index 2658c0a..94ac356 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapperStub.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapperStub.java @@ -86,6 +86,11 @@ public class MetricsRegionWrapperStub implements MetricsRegionWrapper { } @Override + public int getRegionHashCode() { + return 42; + } + + @Override public Map getCoprocessorExecutionStatistics() { return new HashMap(); } -- 2.4.3