From 3e5e9b4b07956b0200bf16b7b0a17567f20dd5fd Mon Sep 17 00:00:00 2001 From: Duo Zhang Date: Wed, 8 Sep 2021 19:38:03 +0800 Subject: [PATCH] HBASE-26248 Should find a suitable way to let users specify the store file tracker implementation --- .../MigrationStoreFileTracker.java | 4 +- .../StoreFileTrackerFactory.java | 70 ++++++++++++++++--- .../TestMigrationStoreFileTracker.java | 23 +++--- ...stRegionWithFileBasedStoreFileTracker.java | 3 +- .../TestStoreFileTrackerFactory.java | 58 +++++++++++++++ 5 files changed, 135 insertions(+), 23 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTrackerFactory.java diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java index e486e6d563af..e252c0c9b657 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java @@ -43,8 +43,8 @@ class MigrationStoreFileTracker extends StoreFileTrackerBase { public MigrationStoreFileTracker(Configuration conf, boolean isPrimaryReplica, StoreContext ctx) { super(conf, isPrimaryReplica, ctx); - this.src = StoreFileTrackerFactory.create(conf, SRC_IMPL, isPrimaryReplica, ctx); - this.dst = StoreFileTrackerFactory.create(conf, DST_IMPL, isPrimaryReplica, ctx); + this.src = StoreFileTrackerFactory.createForMigration(conf, SRC_IMPL, isPrimaryReplica, ctx); + this.dst = StoreFileTrackerFactory.createForMigration(conf, DST_IMPL, isPrimaryReplica, ctx); Preconditions.checkArgument(!src.getClass().equals(dst.getClass()), "src and dst is the same: %s", src.getClass()); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java index 2c2b71db6f8c..db86b246de01 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java @@ -34,27 +34,66 @@ /** * Factory method for creating store file tracker. + *

+ * The current implementations are: + *

+ * @see DefaultStoreFileTracker + * @see FileBasedStoreFileTracker + * @see MigrationStoreFileTracker */ @InterfaceAudience.Private public final class StoreFileTrackerFactory { public static final String TRACK_IMPL = "hbase.store.file-tracker.impl"; private static final Logger LOG = LoggerFactory.getLogger(StoreFileTrackerFactory.class); + /** + * Maps between configuration names for trackers and implementation classes. + */ + public enum Trackers { + DEFAULT(DefaultStoreFileTracker.class), FILE(FileBasedStoreFileTracker.class), + MIGRATION(MigrationStoreFileTracker.class); + + final Class clazz; + + private Trackers(Class clazz) { + this.clazz = clazz; + } + } + + private static Class getTrackerClass(Configuration conf, + String configName) { + try { + Trackers tracker = + Trackers.valueOf(conf.get(TRACK_IMPL, Trackers.DEFAULT.name()).toUpperCase()); + return tracker.clazz; + } catch (IllegalArgumentException e) { + // Fall back to them specifying a class name + return conf.getClass(TRACK_IMPL, Trackers.DEFAULT.clazz, StoreFileTracker.class); + } + } + public static StoreFileTracker create(Configuration conf, boolean isPrimaryReplica, StoreContext ctx) { - Class tracker = - conf.getClass(TRACK_IMPL, DefaultStoreFileTracker.class, StoreFileTracker.class); + Class tracker = getTrackerClass(conf, TRACK_IMPL); LOG.info("instantiating StoreFileTracker impl {}", tracker.getName()); return ReflectionUtils.newInstance(tracker, conf, isPrimaryReplica, ctx); } + /** + * Used at master side when splitting/merging regions, as we do not have a Store, thus no + * StoreContext at master side. + */ public static StoreFileTracker create(Configuration conf, boolean isPrimaryReplica, String family, HRegionFileSystem regionFs) { ColumnFamilyDescriptorBuilder fDescBuilder = ColumnFamilyDescriptorBuilder.newBuilder(Bytes.toBytes(family)); StoreContext ctx = StoreContext.getBuilder().withColumnFamilyDescriptor(fDescBuilder.build()) .withRegionFileSystem(regionFs).build(); - return StoreFileTrackerFactory.create(conf, TRACK_IMPL, isPrimaryReplica, ctx); + return StoreFileTrackerFactory.create(conf, isPrimaryReplica, ctx); } public static Configuration mergeConfigurations(Configuration global, TableDescriptor table, @@ -63,15 +102,30 @@ public static Configuration mergeConfigurations(Configuration global, TableDescr .addStringMap(family.getConfiguration()).addBytesMap(family.getValues()); } - static StoreFileTrackerBase create(Configuration conf, String configName, + /** + * Create store file tracker to be used as source or destination for + * {@link MigrationStoreFileTracker}. + */ + static StoreFileTrackerBase createForMigration(Configuration conf, String configName, boolean isPrimaryReplica, StoreContext ctx) { - String className = + String trackerName = Preconditions.checkNotNull(conf.get(configName), "config %s is not set", configName); Class tracker; try { - tracker = Class.forName(className).asSubclass(StoreFileTrackerBase.class); - } catch (ClassNotFoundException e) { - throw new RuntimeException(e); + tracker = + Trackers.valueOf(trackerName.toUpperCase()).clazz.asSubclass(StoreFileTrackerBase.class); + } catch (IllegalArgumentException e) { + // Fall back to them specifying a class name + try { + tracker = Class.forName(trackerName).asSubclass(StoreFileTrackerBase.class); + } catch (ClassNotFoundException cnfe) { + throw new RuntimeException(cnfe); + } + } + // prevent nest of MigrationStoreFileTracker, it will cause infinite recursion. + if (MigrationStoreFileTracker.class.isAssignableFrom(tracker)) { + throw new IllegalArgumentException("Should not specify " + configName + " as " + + Trackers.MIGRATION + " because it can not be nested"); } LOG.info("instantiating StoreFileTracker impl {} as {}", tracker.getName(), configName); return ReflectionUtils.newInstance(tracker, conf, isPrimaryReplica, ctx); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestMigrationStoreFileTracker.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestMigrationStoreFileTracker.java index e9ebb4c792e4..6cad7a7aa1e1 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestMigrationStoreFileTracker.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestMigrationStoreFileTracker.java @@ -23,7 +23,6 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import java.util.stream.Collectors; import org.apache.hadoop.conf.Configuration; @@ -86,10 +85,10 @@ public class TestMigrationStoreFileTracker { public TestName name = new TestName(); @Parameter(0) - public Class srcImplClass; + public StoreFileTrackerFactory.Trackers srcImpl; @Parameter(1) - public Class dstImplClass; + public StoreFileTrackerFactory.Trackers dstImpl; private HRegion region; @@ -99,11 +98,13 @@ public class TestMigrationStoreFileTracker { @Parameters(name = "{index}: src={0}, dst={1}") public static List params() { - List> impls = - Arrays.asList(DefaultStoreFileTracker.class, FileBasedStoreFileTracker.class); List params = new ArrayList<>(); - for (Class src : impls) { - for (Class dst : impls) { + for (StoreFileTrackerFactory.Trackers src : StoreFileTrackerFactory.Trackers.values()) { + for (StoreFileTrackerFactory.Trackers dst : StoreFileTrackerFactory.Trackers.values()) { + if (src == StoreFileTrackerFactory.Trackers.MIGRATION + || dst == StoreFileTrackerFactory.Trackers.MIGRATION) { + continue; + } if (src.equals(dst)) { continue; } @@ -122,8 +123,8 @@ public static void setUpBeforeClass() { @Before public void setUp() throws IOException { Configuration conf = UTIL.getConfiguration(); - conf.setClass(MigrationStoreFileTracker.SRC_IMPL, srcImplClass, StoreFileTrackerBase.class); - conf.setClass(MigrationStoreFileTracker.DST_IMPL, dstImplClass, StoreFileTrackerBase.class); + conf.set(MigrationStoreFileTracker.SRC_IMPL, srcImpl.name().toLowerCase()); + conf.set(MigrationStoreFileTracker.DST_IMPL, dstImpl.name().toLowerCase()); rootDir = UTIL.getDataTestDir(name.getMethodName().replaceAll("[=:\\[ ]", "_")); wal = HBaseTestingUtil.createWal(conf, rootDir, RI); } @@ -180,14 +181,14 @@ private void verifyData(int start, int end) throws IOException { @Test public void testMigration() throws IOException { - region = createRegion(srcImplClass); + region = createRegion(srcImpl.clazz.asSubclass(StoreFileTrackerBase.class)); putData(0, 100); verifyData(0, 100); reopenRegion(MigrationStoreFileTracker.class); verifyData(0, 100); region.compact(true); putData(100, 200); - reopenRegion(dstImplClass); + reopenRegion(dstImpl.clazz.asSubclass(StoreFileTrackerBase.class)); verifyData(0, 200); } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestRegionWithFileBasedStoreFileTracker.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestRegionWithFileBasedStoreFileTracker.java index 3bc60d13fd94..42d75785a87d 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestRegionWithFileBasedStoreFileTracker.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestRegionWithFileBasedStoreFileTracker.java @@ -71,8 +71,7 @@ public class TestRegionWithFileBasedStoreFileTracker { @Before public void setUp() throws IOException { Configuration conf = new Configuration(UTIL.getConfiguration()); - conf.setClass(StoreFileTrackerFactory.TRACK_IMPL, FileBasedStoreFileTracker.class, - StoreFileTracker.class); + conf.set(StoreFileTrackerFactory.TRACK_IMPL, StoreFileTrackerFactory.Trackers.FILE.name()); region = HBaseTestingUtil.createRegionAndWAL(RI, UTIL.getDataTestDir(name.getMethodName()), conf, TD); } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTrackerFactory.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTrackerFactory.java new file mode 100644 index 000000000000..41f2afdfa421 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTrackerFactory.java @@ -0,0 +1,58 @@ +/** + * 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.storefiletracker; + +import static org.junit.Assert.assertThrows; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseConfiguration; +import org.apache.hadoop.hbase.regionserver.StoreContext; +import org.apache.hadoop.hbase.testclassification.RegionServerTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category({ RegionServerTests.class, SmallTests.class }) +public class TestStoreFileTrackerFactory { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestStoreFileTrackerFactory.class); + + @Test + public void testCreateForMigration() { + Configuration conf = HBaseConfiguration.create(); + String configName = "config"; + + // no config + assertThrows(NullPointerException.class, () -> StoreFileTrackerFactory.createForMigration(conf, + configName, false, StoreContext.getBuilder().build())); + + // class not found + conf.set(configName, "config"); + assertThrows(RuntimeException.class, () -> StoreFileTrackerFactory.createForMigration(conf, + configName, false, StoreContext.getBuilder().build())); + + // nested MigrationStoreFileTracker + conf.setClass(configName, MigrationStoreFileTracker.class, StoreFileTrackerBase.class); + assertThrows(IllegalArgumentException.class, () -> StoreFileTrackerFactory + .createForMigration(conf, configName, false, StoreContext.getBuilder().build())); + } +}