From 4a0b03bec5d861fc8ddc7cfc7538aed1f0edb3f9 Mon Sep 17 00:00:00 2001 From: "Jain, Nihal" Date: Fri, 14 Jul 2023 18:46:05 +0530 Subject: [PATCH 1/3] HBASE-27966 HBase Master/RS JVM metrics populated incorrectly --- .../org/apache/hadoop/hbase/io/MetricsIO.java | 15 ++- .../apache/hadoop/hbase/io/hfile/HFile.java | 10 +- .../hbase/regionserver/TestMetricsJvm.java | 113 ++++++++++++++++++ 3 files changed, 130 insertions(+), 8 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsJvm.java diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java index c2197cef9457..7bdb369ac3ce 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java @@ -24,10 +24,12 @@ @InterfaceAudience.Private public class MetricsIO { + private static MetricsIO instance; private final MetricsIOSource source; private final MetricsIOWrapper wrapper; - public MetricsIO(MetricsIOWrapper wrapper) { + // visible for testing only + MetricsIO(MetricsIOWrapper wrapper) { this(CompatibilitySingletonFactory.getInstance(MetricsRegionServerSourceFactory.class) .createIO(wrapper), wrapper); } @@ -37,6 +39,17 @@ public MetricsIO(MetricsIOWrapper wrapper) { this.wrapper = wrapper; } + /** + * Get a static instance for the MetricsIO so that accessors access the same instance + */ + public static MetricsIO getInstance() { + if (instance != null) { + return instance; + } + instance = new MetricsIO(new MetricsIOWrapperImpl()); + return instance; + } + public MetricsIOSource getMetricsSource() { return source; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java index 73346e8ae4ac..207c99866511 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFile.java @@ -37,7 +37,6 @@ import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.io.FSDataInputStreamWrapper; import org.apache.hadoop.hbase.io.MetricsIO; -import org.apache.hadoop.hbase.io.MetricsIOWrapperImpl; import org.apache.hadoop.hbase.io.compress.Compression; import org.apache.hadoop.hbase.io.encoding.DataBlockEncoding; import org.apache.hadoop.hbase.io.hfile.ReaderContext.ReaderType; @@ -168,9 +167,6 @@ public final class HFile { // For tests. Gets incremented when we read a block whether from HDFS or from Cache. public static final LongAdder DATABLOCK_READ_COUNT = new LongAdder(); - /** Static instance for the metrics so that HFileReaders access the same instance */ - static final MetricsIO metrics = new MetricsIO(new MetricsIOWrapperImpl()); - /** * Shutdown constructor. */ @@ -193,14 +189,14 @@ public static final long getChecksumFailuresCount() { public static final void updateReadLatency(long latencyMillis, boolean pread) { if (pread) { - metrics.updateFsPreadTime(latencyMillis); + MetricsIO.getInstance().updateFsPreadTime(latencyMillis); } else { - metrics.updateFsReadTime(latencyMillis); + MetricsIO.getInstance().updateFsReadTime(latencyMillis); } } public static final void updateWriteLatency(long latencyMillis) { - metrics.updateFsWriteTime(latencyMillis); + MetricsIO.getInstance().updateFsWriteTime(latencyMillis); } /** API required to write an {@link HFile} */ diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsJvm.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsJvm.java new file mode 100644 index 000000000000..6c1ec1dc0acb --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsJvm.java @@ -0,0 +1,113 @@ +/* + * 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 static junit.framework.TestCase.assertFalse; +import static junit.framework.TestCase.assertTrue; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; + +import java.net.HttpURLConnection; +import java.net.URL; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseTestingUtil; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.testclassification.MetricsTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.apache.hadoop.hbase.util.Pair; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClients; +import org.apache.http.util.EntityUtils; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category({ MetricsTests.class, SmallTests.class }) +public class TestMetricsJvm { + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestMetricsJvm.class); + + private final static HBaseTestingUtil UTIL = new HBaseTestingUtil(); + private static Configuration conf; + + @BeforeClass + public static void before() throws Exception { + conf = UTIL.getConfiguration(); + // The master info server does not run in tests by default. + // Set it to ephemeral port so that it will start + conf.setInt(HConstants.MASTER_INFO_PORT, 0); + UTIL.startMiniCluster(); + } + + @AfterClass + public static void after() throws Exception { + UTIL.shutdownMiniCluster(); + } + + @Test + public void testJvmMetrics() throws Exception { + final Pair jmxPage = getJmxPage("?qry=Hadoop:service=HBase,name=JvmMetrics*"); + assertNotNull(jmxPage); + + final Integer responseCode = jmxPage.getFirst(); + final String responseBody = jmxPage.getSecond(); + + assertEquals(HttpURLConnection.HTTP_OK, responseCode.intValue()); + assertNotNull(responseBody); + + assertNotFind("\"tag.ProcessName\"\\s*:\\s*\"IO\"", responseBody); + assertReFind("\"tag.ProcessName\"\\s*:\\s*\"Master\"", responseBody); + } + + private Pair getJmxPage(String query) throws Exception { + URL url = new URL("http://localhost:" + + UTIL.getHBaseCluster().getMaster().getInfoServer().getPort() + "/jmx" + query); + return getUrlContent(url); + } + + private Pair getUrlContent(URL url) throws Exception { + try (CloseableHttpClient client = HttpClients.createDefault()) { + CloseableHttpResponse resp = client.execute(new HttpGet(url.toURI())); + int code = resp.getStatusLine().getStatusCode(); + if (code == HttpURLConnection.HTTP_OK) { + return new Pair<>(code, EntityUtils.toString(resp.getEntity())); + } + return new Pair<>(code, null); + } + } + + private void assertReFind(String re, String value) { + Pattern p = Pattern.compile(re); + Matcher m = p.matcher(value); + assertTrue("'" + p + "' does not match " + value, m.find()); + } + + private void assertNotFind(String re, String value) { + Pattern p = Pattern.compile(re); + Matcher m = p.matcher(value); + assertFalse("'" + p + "' should not match " + value, m.find()); + } +} From f4ec899a94e8dac5f439ceb6e31173a82f21224a Mon Sep 17 00:00:00 2001 From: "Jain, Nihal" Date: Fri, 21 Jul 2023 12:36:01 +0530 Subject: [PATCH 2/3] Change to lazy initialization with double check locking --- .../java/org/apache/hadoop/hbase/io/MetricsIO.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java index 7bdb369ac3ce..138c6273c5b1 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java @@ -24,7 +24,7 @@ @InterfaceAudience.Private public class MetricsIO { - private static MetricsIO instance; + private static volatile MetricsIO instance; private final MetricsIOSource source; private final MetricsIOWrapper wrapper; @@ -40,13 +40,17 @@ public class MetricsIO { } /** - * Get a static instance for the MetricsIO so that accessors access the same instance + * Get a static instance for the MetricsIO so that accessors access the same instance. We want to + * lazy initialize so that correct process name is in place. See HBASE-27966 for more details. */ public static MetricsIO getInstance() { - if (instance != null) { - return instance; + if (instance == null) { + synchronized (MetricsIO.class) { + if (instance == null) { + instance = new MetricsIO(new MetricsIOWrapperImpl()); + } + } } - instance = new MetricsIO(new MetricsIOWrapperImpl()); return instance; } From fa5446df7c135b63f98b5619a8647e2582743adf Mon Sep 17 00:00:00 2001 From: "Jain, Nihal" Date: Wed, 16 Aug 2023 17:48:40 +0530 Subject: [PATCH 3/3] Use RestrictedApi to restrict the place where we can call org.apache.hadoop.hbase.io.MetricsIO#MetricsIO(org.apache.hadoop.hbase.io.MetricsIOWrapper) --- .../src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java index 138c6273c5b1..58e6f7d01b71 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/MetricsIO.java @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hbase.io; +import com.google.errorprone.annotations.RestrictedApi; import org.apache.hadoop.hbase.CompatibilitySingletonFactory; import org.apache.hadoop.hbase.regionserver.MetricsRegionServerSourceFactory; import org.apache.yetus.audience.InterfaceAudience; @@ -28,7 +29,8 @@ public class MetricsIO { private final MetricsIOSource source; private final MetricsIOWrapper wrapper; - // visible for testing only + @RestrictedApi(explanation = "Should only be called in TestMetricsIO", link = "", + allowedOnPath = ".*/(MetricsIO|TestMetricsIO).java") MetricsIO(MetricsIOWrapper wrapper) { this(CompatibilitySingletonFactory.getInstance(MetricsRegionServerSourceFactory.class) .createIO(wrapper), wrapper);