ndimiduk commented on code in PR #7481:
URL: https://github.com/apache/hbase/pull/7481#discussion_r2576204577
##########
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapper.java:
##########
@@ -166,4 +166,13 @@ public interface MetricsRegionWrapper {
/** Returns the number of row reads on memstore and file per store */
Map<String, Long> getMixedRowReadsCount();
+ /**
+ * Returns a SHA-256 hash of the table descriptor that this region was
opened with. This hash
Review Comment:
For something emitted via a metric tag, every character counts. Sha256 in
hex is 64 characters long. Can we get this down to 4 or 8 characters?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapperImpl.java:
##########
@@ -357,6 +364,32 @@ public void run() {
}
}
+ @Override
+ public String getTableDescriptorHash() {
+ return tableDescriptorHash;
+ }
+
+ private String computeTableDescriptorHash() {
+ try {
+ TableDescriptor tableDesc = this.region.getTableDescriptor();
Review Comment:
Should this be computed on the TableDescriptor object itself?
##########
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapper.java:
##########
@@ -166,4 +166,13 @@ public interface MetricsRegionWrapper {
/** Returns the number of row reads on memstore and file per store */
Map<String, Long> getMixedRowReadsCount();
+ /**
+ * Returns a SHA-256 hash of the table descriptor that this region was
opened with. This hash
Review Comment:
Why did you choose SHA-256? I don't think this use-case demands a
cryptographically secure signature. Something cheaper is (probably) fine here
-- crc32 maybe. I haven't looked at which algorithms are provided by the JVM,
but ideally we don't need an external dependency for this.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsRegionWrapperTableDescriptorHash.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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 org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.client.RegionInfoBuilder;
+import org.apache.hadoop.hbase.client.TableDescriptor;
+import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
+import org.apache.hadoop.hbase.testclassification.RegionServerTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category({ RegionServerTests.class, SmallTests.class })
+public class TestMetricsRegionWrapperTableDescriptorHash {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+
HBaseClassTestRule.forClass(TestMetricsRegionWrapperTableDescriptorHash.class);
+
+ private HBaseTestingUtil testUtil;
+ private Configuration conf;
+
+ @Before
+ public void setUp() throws Exception {
+ conf = HBaseConfiguration.create();
+ testUtil = new HBaseTestingUtil(conf);
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ if (testUtil != null) {
+ testUtil.cleanupTestDir();
+ }
+ }
+
+ @Test
+ public void testTableDescriptorHashGeneration() throws Exception {
+ TableName tableName = TableName.valueOf("testTable");
+ TableDescriptor tableDescriptor =
TableDescriptorBuilder.newBuilder(tableName)
+ .setColumnFamily(ColumnFamilyDescriptorBuilder.of("cf")).build();
+
+ RegionInfo regionInfo =
RegionInfoBuilder.newBuilder(tableName).setStartKey(Bytes.toBytes("a"))
+ .setEndKey(Bytes.toBytes("z")).build();
+
+ Path testDir =
testUtil.getDataTestDir("testTableDescriptorHashGeneration");
+ HRegion region =
+ HBaseTestingUtil.createRegionAndWAL(regionInfo, testDir, conf,
tableDescriptor);
+
+ MetricsRegionWrapperImpl wrapper = new MetricsRegionWrapperImpl(region);
+
+ String hash = wrapper.getTableDescriptorHash();
+ assertNotNull(hash);
+ assertNotEquals("unknown", hash);
+ assertEquals(64, hash.length());
+
+ wrapper.close();
+ HBaseTestingUtil.closeRegionAndWAL(region);
+ }
+
+ @Test
+ public void testHashConsistency() throws Exception {
+ TableName tableName = TableName.valueOf("testTable2");
+ TableDescriptor tableDescriptor =
TableDescriptorBuilder.newBuilder(tableName)
+ .setColumnFamily(ColumnFamilyDescriptorBuilder.of("cf")).build();
+
+ RegionInfo regionInfo1 =
RegionInfoBuilder.newBuilder(tableName).setStartKey(Bytes.toBytes("a"))
+ .setEndKey(Bytes.toBytes("m")).build();
+ RegionInfo regionInfo2 =
RegionInfoBuilder.newBuilder(tableName).setStartKey(Bytes.toBytes("m"))
+ .setEndKey(Bytes.toBytes("z")).build();
+
+ Path testDir1 = testUtil.getDataTestDir("testHashConsistency1");
+ HRegion region1 =
+ HBaseTestingUtil.createRegionAndWAL(regionInfo1, testDir1, conf,
tableDescriptor);
+
+ Path testDir2 = testUtil.getDataTestDir("testHashConsistency2");
+ HRegion region2 =
+ HBaseTestingUtil.createRegionAndWAL(regionInfo2, testDir2, conf,
tableDescriptor);
+
+ MetricsRegionWrapperImpl wrapper1 = new MetricsRegionWrapperImpl(region1);
+ MetricsRegionWrapperImpl wrapper2 = new MetricsRegionWrapperImpl(region2);
+
+ String hash1 = wrapper1.getTableDescriptorHash();
+ String hash2 = wrapper2.getTableDescriptorHash();
+
+ assertEquals(hash1, hash2);
+
+ wrapper1.close();
Review Comment:
Can you use try-with-resources and/or a finally block?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsRegionWrapperImpl.java:
##########
@@ -357,6 +364,32 @@ public void run() {
}
}
+ @Override
+ public String getTableDescriptorHash() {
+ return tableDescriptorHash;
+ }
+
+ private String computeTableDescriptorHash() {
+ try {
+ TableDescriptor tableDesc = this.region.getTableDescriptor();
Review Comment:
Wrap the computation in an LRU cache to avoid recomputing this 100's of
times for all regions of the same table open on one host?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
