HBASE-26248 Should find a suitable way to let users specify the store file tracker implementation

Author: Duo Zhang

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());
   }

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java

@@ -34,27 +34,66 @@
 
 /**
  * Factory method for creating store file tracker.
+ * <p/>
+ * The current implementations are:
+ * <ul>
+ * <li><em>default</em>: DefaultStoreFileTracker, see {@link DefaultStoreFileTracker}.</li>
+ * <li><em>file</em>:FileBasedStoreFileTracker, see {@link FileBasedStoreFileTracker}.</li>
+ * <li><em>migration</em>:MigrationStoreFileTracker, see {@link MigrationStoreFileTracker}.</li>
+ * </ul>
+ * @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<? extends StoreFileTracker> clazz;
+
+    private Trackers(Class<? extends StoreFileTracker> clazz) {
+      this.clazz = clazz;
+    }
+  }
+
+  private static Class<? extends StoreFileTracker> 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<? extends StoreFileTracker> tracker =
-      conf.getClass(TRACK_IMPL, DefaultStoreFileTracker.class, StoreFileTracker.class);
+    Class<? extends StoreFileTracker> 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<? extends StoreFileTrackerBase> 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);

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<? extends StoreFileTrackerBase> srcImplClass;
+  public StoreFileTrackerFactory.Trackers srcImpl;
 
   @Parameter(1)
-  public Class<? extends StoreFileTrackerBase> 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<Object[]> params() {
-    List<Class<? extends StoreFileTrackerBase>> impls =
-      Arrays.asList(DefaultStoreFileTracker.class, FileBasedStoreFileTracker.class);
     List<Object[]> params = new ArrayList<>();
-    for (Class<? extends StoreFileTrackerBase> src : impls) {
-      for (Class<? extends StoreFileTrackerBase> 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);
   }
 }

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);
   }

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()));
+  }
+}