From 6211df6a57e3cfddb3ff06ff091c1e72c6eeadb4 Mon Sep 17 00:00:00 2001 From: Karen Coppage Date: Tue, 27 Jul 2021 09:35:59 +0200 Subject: [PATCH] HIVE-25390: Metrics compaction_failed_initiator_ratio and compaction_failed_cleaner_ratio should be counters --- .../hadoop/hive/ql/txn/compactor/Cleaner.java | 12 ++---- .../hive/ql/txn/compactor/Initiator.java | 10 ++--- .../txn/compactor/TestCompactionMetrics.java | 39 +++++++------------ .../hive/metastore/metrics/Metrics.java | 30 -------------- .../metastore/metrics/MetricsConstants.java | 4 +- 5 files changed, 22 insertions(+), 73 deletions(-) diff --git a/ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java b/ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java index 988455f455c4..ffff94489979 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Cleaner.java @@ -17,7 +17,6 @@ */ package org.apache.hadoop.hive.ql.txn.compactor; -import org.apache.commons.lang3.tuple.Pair; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hive.common.FileUtils; import org.apache.hadoop.hive.common.TableName; @@ -62,7 +61,6 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicInteger; import static org.apache.hadoop.hive.conf.Constants.COMPACTOR_CLEANER_THREAD_NAME_FORMAT; import static org.apache.hadoop.hive.conf.HiveConf.ConfVars.HIVE_COMPACTOR_CLEANER_RETENTION_TIME; @@ -70,6 +68,8 @@ import static org.apache.hadoop.hive.metastore.HMSHandler.getMSForConf; import static org.apache.hadoop.hive.metastore.utils.MetaStoreUtils.getDefaultCatalog; +import com.codahale.metrics.Counter; + /** * A class to clean directories after compactions. This will run in a separate thread. */ @@ -98,8 +98,7 @@ public void run() { try { boolean metricsEnabled = MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METRICS_ENABLED) && MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON); - Pair ratio = - Metrics.getOrCreateRatio(MetricsConstants.COMPACTION_FAILED_CLEANER_RATIO); + Counter failuresCounter = Metrics.getOrCreateCounter(MetricsConstants.COMPACTION_CLEANER_FAILURE_COUNTER); do { TxnStore.MutexAPI.LockHandle handle = null; long startedAt = -1; @@ -112,9 +111,6 @@ public void run() { // so wrap it in a big catch Throwable statement. try { handle = txnHandler.getMutexAPI().acquireLock(TxnStore.MUTEX_KEY.Cleaner.name()); - if (metricsEnabled) { - ratio.getRight().incrementAndGet(); - } startedAt = System.currentTimeMillis(); long minOpenTxnId = txnHandler.findMinOpenTxnIdForCleaner(); @@ -140,7 +136,7 @@ public void run() { } catch (Throwable t) { // the lock timeout on AUX lock, should be ignored. if (metricsEnabled && handle != null) { - ratio.getLeft().incrementAndGet(); + failuresCounter.inc(); } LOG.error("Caught an exception in the main loop of compactor cleaner, " + StringUtils.stringifyException(t)); diff --git a/ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java b/ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java index 63b2a3018292..1b3e964345dc 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/txn/compactor/Initiator.java @@ -17,9 +17,9 @@ */ package org.apache.hadoop.hive.ql.txn.compactor; +import com.codahale.metrics.Counter; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Sets; -import org.apache.commons.lang3.tuple.Pair; import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; @@ -64,7 +64,6 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -72,7 +71,6 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; import static org.apache.hadoop.hive.conf.Constants.COMPACTOR_INTIATOR_THREAD_NAME_FORMAT; @@ -107,8 +105,7 @@ public void run() { TimeUnit.MILLISECONDS); boolean metricsEnabled = MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METRICS_ENABLED) && MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON); - Pair ratio = - Metrics.getOrCreateRatio(MetricsConstants.COMPACTION_FAILED_INITIATOR_RATIO); + Counter failuresCounter = Metrics.getOrCreateCounter(MetricsConstants.COMPACTION_INITIATOR_FAILURE_COUNTER); // Make sure we run through the loop once before checking to stop as this makes testing // much easier. The stop value is only for testing anyway and not used when called from @@ -123,7 +120,6 @@ public void run() { try { handle = txnHandler.getMutexAPI().acquireLock(TxnStore.MUTEX_KEY.Initiator.name()); if (metricsEnabled) { - ratio.getRight().incrementAndGet(); perfLogger.perfLogBegin(CLASS_NAME, MetricsConstants.COMPACTION_INITIATOR_CYCLE); } startedAt = System.currentTimeMillis(); @@ -182,7 +178,7 @@ public void run() { } catch (Throwable t) { // the lock timeout on AUX lock, should be ignored. if (metricsEnabled && handle != null) { - ratio.getLeft().incrementAndGet(); + failuresCounter.inc(); } LOG.error("Initiator loop caught unexpected exception this time through the loop: " + StringUtils.stringifyException(t)); diff --git a/ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java b/ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java index 5104874714f7..3f40b3fcc800 100644 --- a/ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java +++ b/ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCompactionMetrics.java @@ -17,7 +17,7 @@ */ package org.apache.hadoop.hive.ql.txn.compactor; -import org.apache.commons.lang3.tuple.Pair; +import com.codahale.metrics.Counter; import com.google.common.collect.Maps; import org.apache.hadoop.hive.common.ServerUtils; import org.apache.hadoop.hive.common.metrics.MetricsTestUtils; @@ -229,19 +229,15 @@ public void testOldestReadyForCleaningAge() throws Exception { @Test public void testInitiatorNoFailure() throws Exception { startInitiator(); - Pair ratio = - Metrics.getOrCreateRatio(MetricsConstants.COMPACTION_FAILED_INITIATOR_RATIO); - Assert.assertEquals("numerator mismatch", 0, ratio.getLeft().get()); - Assert.assertEquals("denominator mismatch", 1, ratio.getRight().get()); + Counter counter = Metrics.getOrCreateCounter(MetricsConstants.COMPACTION_INITIATOR_FAILURE_COUNTER); + Assert.assertEquals("Count incorrect", 0, counter.getCount()); } @Test public void testCleanerNoFailure() throws Exception { startCleaner(); - Pair ratio = - Metrics.getOrCreateRatio(MetricsConstants.COMPACTION_FAILED_CLEANER_RATIO); - Assert.assertEquals("numerator mismatch", 0, ratio.getLeft().get()); - Assert.assertEquals("denominator mismatch", 1, ratio.getRight().get()); + Counter counter = Metrics.getOrCreateCounter(MetricsConstants.COMPACTION_CLEANER_FAILURE_COUNTER); + Assert.assertEquals("Count incorrect", 0, counter.getCount()); } @Test @@ -249,10 +245,8 @@ public void testInitiatorFailure() throws Exception { ThrowingTxnHandler.doThrow = true; MetastoreConf.setVar(conf, MetastoreConf.ConfVars.TXN_STORE_IMPL, "org.apache.hadoop.hive.metastore.txn.ThrowingTxnHandler"); startInitiator(); - Pair ratio = - Metrics.getOrCreateRatio(MetricsConstants.COMPACTION_FAILED_INITIATOR_RATIO); - Assert.assertEquals("numerator mismatch", 1, ratio.getLeft().get()); - Assert.assertEquals("denominator mismatch", 1, ratio.getRight().get()); + Counter counter = Metrics.getOrCreateCounter(MetricsConstants.COMPACTION_INITIATOR_FAILURE_COUNTER); + Assert.assertEquals("Count incorrect", 1, counter.getCount()); } @Test @@ -260,10 +254,8 @@ public void testCleanerFailure() throws Exception { ThrowingTxnHandler.doThrow = true; MetastoreConf.setVar(conf, MetastoreConf.ConfVars.TXN_STORE_IMPL, "org.apache.hadoop.hive.metastore.txn.ThrowingTxnHandler"); startCleaner(); - Pair ratio = - Metrics.getOrCreateRatio(MetricsConstants.COMPACTION_FAILED_CLEANER_RATIO); - Assert.assertEquals("numerator mismatch", 1, ratio.getLeft().get()); - Assert.assertEquals("denominator mismatch", 1, ratio.getRight().get()); + Counter counter = Metrics.getOrCreateCounter(MetricsConstants.COMPACTION_CLEANER_FAILURE_COUNTER); + Assert.assertEquals("Count incorrect", 1, counter.getCount()); } @Test @@ -280,11 +272,8 @@ public void testInitiatorAuxFailure() throws Exception { } } // the lock timeout on AUX lock, should be ignored. - Pair ratio = - Metrics.getOrCreateRatio(MetricsConstants.COMPACTION_FAILED_INITIATOR_RATIO); - Assert.assertEquals(0, ratio.getLeft().get()); - Assert.assertEquals("numerator mismatch", 0, ratio.getLeft().get()); - Assert.assertEquals("denominator mismatch", 0, ratio.getRight().get()); + Counter failureCounter = Metrics.getOrCreateCounter(MetricsConstants.COMPACTION_INITIATOR_FAILURE_COUNTER); + Assert.assertEquals("count mismatch", 0, failureCounter.getCount()); } @Test @@ -301,10 +290,8 @@ public void testCleanerAuxFailure() throws Exception { } } // the lock timeout on AUX lock, should be ignored. - Pair ratio = - Metrics.getOrCreateRatio(MetricsConstants.COMPACTION_FAILED_CLEANER_RATIO); - Assert.assertEquals("numerator mismatch", 0, ratio.getLeft().get()); - Assert.assertEquals("denominator mismatch", 0, ratio.getRight().get()); + Counter failureCounter = Metrics.getOrCreateCounter(MetricsConstants.COMPACTION_CLEANER_FAILURE_COUNTER); + Assert.assertEquals("count mismatch", 0, failureCounter.getCount()); } @Test diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/Metrics.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/Metrics.java index 6e09d61d512f..d72cfa649ff3 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/Metrics.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/Metrics.java @@ -32,7 +32,6 @@ import com.codahale.metrics.Metric; import com.codahale.metrics.MetricRegistry; import com.codahale.metrics.MetricSet; -import com.codahale.metrics.RatioGauge; import com.codahale.metrics.Reporter; import com.codahale.metrics.ScheduledReporter; import com.codahale.metrics.Slf4jReporter; @@ -161,35 +160,6 @@ public Integer getValue() { } } - /** - * Get the pair of AtomicIntegers behind an existing ratio gauge, or create a new gauge if it does not already - * exist. - * @param name Name of gauge. This should come from MetricConstants - * @return Pair as the numerator and denominator of the ratio. - */ - public static Pair getOrCreateRatio(String name) { - // We return a garbage value if metrics haven't been initialized so that callers don't have - // to keep checking if the resulting value is null. - if (self == null) return dummyRatio; - Pair ratio = self.gaugeRatio.get(name); - if (ratio != null) return ratio; - synchronized (Metrics.class) { - ratio = self.gaugeRatio.get(name); - if (ratio != null) return ratio; - ratio = Pair.of(new AtomicInteger(), new AtomicInteger()); - final Pair forGauge = ratio; - self.gaugeRatio.put(name, ratio); - self.registry.register(name, new RatioGauge() { - @Override - protected Ratio getRatio() { - return Ratio.of(forGauge.getLeft().get(), forGauge.getRight().get()); - } - }); - return ratio; - } - } - - public static Counter getOpenConnectionsCounter() { return getOrCreateCounter(MetricsConstants.OPEN_CONNECTIONS); } diff --git a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/MetricsConstants.java b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/MetricsConstants.java index 4f4f4075c34b..667668476b17 100644 --- a/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/MetricsConstants.java +++ b/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metrics/MetricsConstants.java @@ -23,9 +23,9 @@ public class MetricsConstants { public static final String COMPACTION_STATUS_PREFIX = "compaction_num_"; public static final String COMPACTION_OLDEST_ENQUEUE_AGE = "compaction_oldest_enqueue_age_in_sec"; public static final String COMPACTION_INITIATOR_CYCLE = "compaction_initiator_cycle"; - public static final String COMPACTION_FAILED_INITIATOR_RATIO = "compaction_failed_initiator_ratio"; + public static final String COMPACTION_INITIATOR_FAILURE_COUNTER = "compaction_initiator_failure_counter"; public static final String COMPACTION_CLEANER_CYCLE = "compaction_cleaner_cycle"; - public static final String COMPACTION_FAILED_CLEANER_RATIO = "compaction_failed_cleaner_ratio"; + public static final String COMPACTION_CLEANER_FAILURE_COUNTER = "compaction_cleaner_failure_counter"; public static final String COMPACTION_WORKER_CYCLE = "compaction_worker_cycle"; public static final String OLDEST_OPEN_REPL_TXN_ID = "oldest_open_repl_txn_id";