HBASE-26248 Should find a suitable way to let users specify the store…

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

@@ -22,6 +22,7 @@
 import java.util.List;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
+import org.apache.hadoop.hbase.procedure2.util.StringUtils;
 import org.apache.hadoop.hbase.regionserver.StoreContext;
 import org.apache.hadoop.hbase.regionserver.StoreFileInfo;
 import org.apache.yetus.audience.InterfaceAudience;
@@ -44,8 +45,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());
   }
@@ -90,7 +91,11 @@ void set(List<StoreFileInfo> files) {
   @Override
   public void persistConfiguration(TableDescriptorBuilder builder) {
     super.persistConfiguration(builder);
-    builder.setValue(SRC_IMPL, src.getClass().getName());
-    builder.setValue(DST_IMPL, dst.getClass().getName());
+    if (StringUtils.isEmpty(builder.getValue(SRC_IMPL))) {
+      builder.setValue(SRC_IMPL, src.getTrackerName());
+    }
+    if (StringUtils.isEmpty(builder.getValue(DST_IMPL))) {
+      builder.setValue(DST_IMPL, dst.getTrackerName());
+    }
   }
 }

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

@@ -75,7 +75,12 @@ void replace(Collection<StoreFileInfo> compactedFiles, Collection<StoreFileInfo>
   StoreFileWriter createWriter(CreateStoreFileWriterParams params) throws IOException;
 
   /**
-   * Saves StoreFileTracker implementations specific configs into the table descriptors.
+   * Saves StoreFileTracker implementations specific configurations into the table descriptors.
+   * <p/>
+   * This is used to avoid accidentally data loss when changing the cluster level store file tracker
+   * implementation, and also possible misconfiguration between master and region servers.
+   * <p/>
+   * See HBASE-26246 for more details.
    * @param builder The table descriptor builder for the given table.
    */
   void persistConfiguration(TableDescriptorBuilder builder);

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

@@ -17,7 +17,7 @@
  */
 package org.apache.hadoop.hbase.regionserver.storefiletracker;
 
-import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACK_IMPL;
+import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACKER_IMPL;
 
 import java.io.IOException;
 import java.util.Collection;
@@ -84,13 +84,15 @@ public final void replace(Collection<StoreFileInfo> compactedFiles,
 
   @Override
   public void persistConfiguration(TableDescriptorBuilder builder) {
-    if (StringUtils.isEmpty(builder.getValue(TRACK_IMPL))) {
-      String trackerImpl = StoreFileTrackerFactory.
-        getStoreFileTrackerImpl(conf).getName();
-      builder.setValue(TRACK_IMPL, trackerImpl).build();
+    if (StringUtils.isEmpty(builder.getValue(TRACKER_IMPL))) {
+      builder.setValue(TRACKER_IMPL, getTrackerName());
     }
   }
 
+  protected final String getTrackerName() {
+    return StoreFileTrackerFactory.getStoreFileTrackerName(getClass());
+  }
+
   private HFileContext createFileContext(Compression.Algorithm compression,
     boolean includeMVCCReadpoint, boolean includesTag, Encryption.Context encryptionContext) {
     if (compression == null) {

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

@@ -15,6 +15,9 @@
  */
 package org.apache.hadoop.hbase.regionserver.storefiletracker;
 
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.CompoundConfiguration;
 import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
@@ -33,22 +36,81 @@
 
 /**
  * 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";
+@InterfaceAudience.Private
+public final class StoreFileTrackerFactory {
+
   private static final Logger LOG = LoggerFactory.getLogger(StoreFileTrackerFactory.class);
 
-  public static Class<? extends StoreFileTracker> getStoreFileTrackerImpl(Configuration conf) {
-    return conf.getClass(TRACK_IMPL, DefaultStoreFileTracker.class, StoreFileTracker.class);
+  public static final String TRACKER_IMPL = "hbase.store.file-tracker.impl";
+
+  /**
+   * 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;
+
+    Trackers(Class<? extends StoreFileTracker> clazz) {
+      this.clazz = clazz;
+    }
+  }
+
+  private static final Map<Class<? extends StoreFileTracker>, Trackers> CLASS_TO_ENUM = reverse();
+
+  private static Map<Class<? extends StoreFileTracker>, Trackers> reverse() {
+    Map<Class<? extends StoreFileTracker>, Trackers> map = new HashMap<>();
+    for (Trackers tracker : Trackers.values()) {
+      map.put(tracker.clazz, tracker);
+    }
+    return Collections.unmodifiableMap(map);
+  }
+
+  private StoreFileTrackerFactory() {
+  }
+
+  public static String getStoreFileTrackerName(Configuration conf) {
+    return conf.get(TRACKER_IMPL, Trackers.DEFAULT.name());
+  }
+
+  static String getStoreFileTrackerName(Class<? extends StoreFileTracker> clazz) {
+    Trackers name = CLASS_TO_ENUM.get(clazz);
+    return name != null ? name.name() : clazz.getName();
+  }
+
+  private static Class<? extends StoreFileTracker> getTrackerClass(Configuration conf) {
+    try {
+      Trackers tracker = Trackers.valueOf(getStoreFileTrackerName(conf).toUpperCase());
+      return tracker.clazz;
+    } catch (IllegalArgumentException e) {
+      // Fall back to them specifying a class name
+      return conf.getClass(TRACKER_IMPL, Trackers.DEFAULT.clazz, StoreFileTracker.class);
+    }
   }
 
   public static StoreFileTracker create(Configuration conf, boolean isPrimaryReplica,
     StoreContext ctx) {
-    Class<? extends StoreFileTracker> tracker = getStoreFileTrackerImpl(conf);
+    Class<? extends StoreFileTracker> tracker = getTrackerClass(conf);
     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 =
@@ -64,15 +126,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/client/TestAdmin.java

@@ -18,7 +18,7 @@
 package org.apache.hadoop.hbase.client;
 
 import static org.apache.hadoop.hbase.HBaseTestingUtil.countRows;
-import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACK_IMPL;
+import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACKER_IMPL;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotEquals;
@@ -426,8 +426,8 @@ private void testCloneTableSchema(final TableName tableName, final TableName new
     assertEquals(BLOCK_CACHE, newTableDesc.getColumnFamily(FAMILY_1).isBlockCacheEnabled());
     assertEquals(TTL, newTableDesc.getColumnFamily(FAMILY_1).getTimeToLive());
     // HBASE-26246 introduced persist of store file tracker into table descriptor
-    tableDesc = TableDescriptorBuilder.newBuilder(tableDesc).setValue(TRACK_IMPL,
-      StoreFileTrackerFactory.getStoreFileTrackerImpl(TEST_UTIL.getConfiguration()).getName()).
+    tableDesc = TableDescriptorBuilder.newBuilder(tableDesc).setValue(TRACKER_IMPL,
+      StoreFileTrackerFactory.getStoreFileTrackerName(TEST_UTIL.getConfiguration())).
       build();
     TEST_UTIL.verifyTableDescriptorIgnoreTableName(tableDesc, newTableDesc);
 

hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAdmin3.java

@@ -17,7 +17,7 @@
  */
 package org.apache.hadoop.hbase.client;
 
-import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACK_IMPL;
+import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACKER_IMPL;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -239,8 +239,8 @@ public void testGetTableDescriptor() throws IOException {
     Table table = TEST_UTIL.getConnection().getTable(htd.getTableName());
     TableDescriptor confirmedHtd = table.getDescriptor();
     //HBASE-26246 introduced persist of store file tracker into table descriptor
-    htd = TableDescriptorBuilder.newBuilder(htd).setValue(TRACK_IMPL,
-      StoreFileTrackerFactory.getStoreFileTrackerImpl(TEST_UTIL.getConfiguration()).getName()).
+    htd = TableDescriptorBuilder.newBuilder(htd).setValue(TRACKER_IMPL,
+      StoreFileTrackerFactory.getStoreFileTrackerName(TEST_UTIL.getConfiguration())).
       build();
     assertEquals(0, TableDescriptor.COMPARATOR.compare(htd, confirmedHtd));
     MetaTableAccessor.fullScanMetaAndPrint(TEST_UTIL.getConnection());

hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableAdminApi.java

@@ -18,7 +18,7 @@
 package org.apache.hadoop.hbase.client;
 
 import static org.apache.hadoop.hbase.TableName.META_TABLE_NAME;
-import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACK_IMPL;
+import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACKER_IMPL;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -375,8 +375,8 @@ private void testCloneTableSchema(final TableName tableName,
     assertEquals(BLOCK_CACHE, newTableDesc.getColumnFamily(FAMILY_1).isBlockCacheEnabled());
     assertEquals(TTL, newTableDesc.getColumnFamily(FAMILY_1).getTimeToLive());
     //HBASE-26246 introduced persist of store file tracker into table descriptor
-    tableDesc = TableDescriptorBuilder.newBuilder(tableDesc).setValue(TRACK_IMPL,
-      StoreFileTrackerFactory.getStoreFileTrackerImpl(TEST_UTIL.getConfiguration()).getName()).
+    tableDesc = TableDescriptorBuilder.newBuilder(tableDesc).setValue(TRACKER_IMPL,
+      StoreFileTrackerFactory.getStoreFileTrackerName(TEST_UTIL.getConfiguration())).
       build();
     TEST_UTIL.verifyTableDescriptorIgnoreTableName(tableDesc, newTableDesc);
 

hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableAdminApi3.java

@@ -18,7 +18,7 @@
 package org.apache.hadoop.hbase.client;
 
 import static org.apache.hadoop.hbase.TableName.META_TABLE_NAME;
-import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACK_IMPL;
+import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACKER_IMPL;
 import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
@@ -150,8 +150,8 @@ public void testGetTableDescriptor() throws Exception {
     admin.createTable(desc).join();
     TableDescriptor confirmedHtd = admin.getDescriptor(tableName).get();
     //HBASE-26246 introduced persist of store file tracker into table descriptor
-    desc = TableDescriptorBuilder.newBuilder(desc).setValue(TRACK_IMPL,
-      StoreFileTrackerFactory.getStoreFileTrackerImpl(TEST_UTIL.getConfiguration()).getName()).
+    desc = TableDescriptorBuilder.newBuilder(desc).setValue(TRACKER_IMPL,
+      StoreFileTrackerFactory.getStoreFileTrackerName(TEST_UTIL.getConfiguration())).
       build();
     assertEquals(0, TableDescriptor.COMPARATOR.compare(desc, confirmedHtd));
   }

hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureTestingUtility.java

@@ -18,7 +18,7 @@
 
 package org.apache.hadoop.hbase.master.procedure;
 
-import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACK_IMPL;
+import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACKER_IMPL;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -231,8 +231,8 @@ public static void validateTableCreation(final HMaster master, final TableName t
 
     // checks store file tracker impl has been properly set in htd
     String storeFileTrackerImpl =
-      StoreFileTrackerFactory.getStoreFileTrackerImpl(master.getConfiguration()).getName();
-    assertEquals(storeFileTrackerImpl, htd.getValue(TRACK_IMPL));
+      StoreFileTrackerFactory.getStoreFileTrackerName(master.getConfiguration());
+    assertEquals(storeFileTrackerImpl, htd.getValue(TRACKER_IMPL));
   }
 
   public static void validateTableDeletion(

hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCreateTableProcedure.java

@@ -17,7 +17,7 @@
  */
 package org.apache.hadoop.hbase.master.procedure;
 
-import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACK_IMPL;
+import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACKER_IMPL;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -96,13 +96,13 @@ public void testCreateWithTrackImpl() throws Exception {
     ProcedureExecutor<MasterProcedureEnv> procExec = getMasterProcedureExecutor();
     TableDescriptor htd = MasterProcedureTestingUtility.createHTD(tableName, F1);
     String trackerName = TestStoreFileTracker.class.getName();
-    htd = TableDescriptorBuilder.newBuilder(htd).setValue(TRACK_IMPL, trackerName).build();
+    htd = TableDescriptorBuilder.newBuilder(htd).setValue(TRACKER_IMPL, trackerName).build();
     RegionInfo[] regions = ModifyRegionUtils.createRegionInfos(htd, null);
     long procId = ProcedureTestingUtility.submitAndWait(procExec,
       new CreateTableProcedure(procExec.getEnvironment(), htd, regions));
     ProcedureTestingUtility.assertProcNotFailed(procExec.getResult(procId));
     htd = getMaster().getTableDescriptors().get(tableName);
-    assertEquals(trackerName, htd.getValue(TRACK_IMPL));
+    assertEquals(trackerName, htd.getValue(TRACKER_IMPL));
   }
 
   @Test

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMergesSplitsAddToTracker.java

@@ -18,7 +18,7 @@
 package org.apache.hadoop.hbase.regionserver;
 
 import static org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.
-  TRACK_IMPL;
+  TRACKER_IMPL;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -74,7 +74,7 @@ public class TestMergesSplitsAddToTracker {
 
   @BeforeClass
   public static void setupClass() throws Exception {
-    TEST_UTIL.getConfiguration().set(TRACK_IMPL, TestStoreFileTracker.class.getName());
+    TEST_UTIL.getConfiguration().set(TRACKER_IMPL, TestStoreFileTracker.class.getName());
     TEST_UTIL.startMiniCluster();
   }