[phoenix] branch 5.1 updated: PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in RegionServers

[st...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

stoty pushed a commit to branch 5.1
in repository https://gitbox.apache.org/repos/asf/phoenix.git


The following commit(s) were added to refs/heads/5.1 by this push:
     new 2592cafe2c PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in RegionServers
2592cafe2c is described below

commit 2592cafe2cae2d671e2e24ea35722bcc57456f05
Author: Istvan Toth <st...@apache.org>
AuthorDate: Thu Apr 28 07:21:33 2022 +0200

    PHOENIX-6699 Phoenix metrics overwriting DefaultMetricsSystem in RegionServers
---
 .../monitoring/GlobalMetricRegistriesAdapter.java  |  7 ++++++
 .../org/apache/phoenix/monitoring/MetricUtil.java  | 26 ++++++++++++++++++++++
 .../apache/phoenix/monitoring/MetricUtilTest.java  | 17 ++++++++++++++
 3 files changed, 50 insertions(+)

diff --git a/phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java b/phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java
index 0f1c8c2744..4a9093511f 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/monitoring/GlobalMetricRegistriesAdapter.java
@@ -53,6 +53,13 @@ public class GlobalMetricRegistriesAdapter {
     private static GlobalMetricRegistriesAdapter INSTANCE = new GlobalMetricRegistriesAdapter();
 
     private GlobalMetricRegistriesAdapter() {
+        if (MetricUtil.isDefaultMetricsInitialized()) {
+            // Prevent clobbering the default metrics HBase has set up in
+            // RS or Master while JmxCacheBuster shuts the Metrics down
+            LOGGER.info("HBase metrics is already initialized. "
+                    + "Skipping Phoenix metrics initialization.");
+            return;
+        }
         DefaultMetricsSystem.initialize("Phoenix");
         JvmMetrics.initSingleton("Phoenix", "");
     }
diff --git a/phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java b/phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java
index 1974eb806e..bbe5f2a46e 100644
--- a/phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java
+++ b/phoenix-core/src/main/java/org/apache/phoenix/monitoring/MetricUtil.java
@@ -17,11 +17,19 @@
  */
 package org.apache.phoenix.monitoring;
 
+import java.lang.reflect.Field;
+
+import org.apache.hadoop.metrics2.impl.MetricsSystemImpl;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
 import org.apache.phoenix.log.LogLevel;
 import org.apache.phoenix.monitoring.CombinableMetric.NoOpRequestMetric;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class MetricUtil {
 
+    private static final Logger LOGGER = LoggerFactory.getLogger(MetricUtil.class);
+
     public static CombinableMetric getCombinableMetric(boolean isRequestMetricsEnabled,
                                                        LogLevel connectionLogLevel,
                                                        MetricType type) {
@@ -38,4 +46,22 @@ public class MetricUtil {
         return new MetricsStopWatch(true);
     }
 
+    // We need to cover the case when JmxCacheBuster has just stopped the HBase metrics
+    // system, and not accidentally overwrite the DefaultMetricsSystem singleton.
+    // See PHOENIX-6699
+    public static boolean isDefaultMetricsInitialized() {
+        try {
+            MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+            Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");
+            prefixField.setAccessible(true);
+            String prefix = (String) prefixField.get(metrics);
+            prefixField.setAccessible(false);
+            if (prefix != null) {
+                return true;
+            }
+        } catch (Exception e) {
+            LOGGER.error("Exception trying to determine if HBase metrics is initialized", e);
+        }
+        return false;
+    }
 }
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/monitoring/MetricUtilTest.java b/phoenix-core/src/test/java/org/apache/phoenix/monitoring/MetricUtilTest.java
index 141ce724d1..3e44ecfceb 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/monitoring/MetricUtilTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/monitoring/MetricUtilTest.java
@@ -17,6 +17,8 @@
  */
 package org.apache.phoenix.monitoring;
 
+import org.apache.hadoop.metrics2.impl.MetricsSystemImpl;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
 import org.apache.phoenix.log.LogLevel;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -27,6 +29,8 @@ import static org.apache.phoenix.monitoring.MetricType.WALL_CLOCK_TIME_MS;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
+import java.lang.reflect.Field;
+
 @RunWith(MockitoJUnitRunner.class)
 public class MetricUtilTest {
 
@@ -48,4 +52,17 @@ public class MetricUtilTest {
                 LogLevel.OFF, WALL_CLOCK_TIME_MS);
         assertFalse(metricsStopWatch.getMetricsEnabled());
     }
+
+    @Test
+    //Check that MetricsSystemImpl has a String "prefix" field in the Hadoop version we test with
+    public void testInternalMetricsField() throws NoSuchFieldException,
+            SecurityException, IllegalArgumentException, IllegalAccessException {
+        MetricsSystemImpl metrics = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+        Field prefixField = MetricsSystemImpl.class.getDeclaredField("prefix");
+        prefixField.setAccessible(true);
+        String oldValue = (String)prefixField.get(metrics);
+        prefixField.set(metrics, "dummy");
+        prefixField.set(metrics, oldValue);
+        prefixField.setAccessible(false);
+    }
 }