HBASE-26264 Add more checks to prevent misconfiguration on store file…

… tracker (#3681)

Signed-off-by: Josh Elser <elserj@apache.org>

hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java

@@ -614,8 +614,7 @@ private List<Path> mergeStoreFiles(MasterProcedureEnv env, HRegionFileSystem reg
       String family = hcd.getNameAsString();
       Configuration trackerConfig =
         StoreFileTrackerFactory.mergeConfigurations(env.getMasterConfiguration(), htd, hcd);
-      StoreFileTracker tracker = StoreFileTrackerFactory.create(trackerConfig, true,
-        family, regionFs);
+      StoreFileTracker tracker = StoreFileTrackerFactory.create(trackerConfig, family, regionFs);
       final Collection<StoreFileInfo> storeFiles = tracker.load();
       if (storeFiles != null && storeFiles.size() > 0) {
         final Configuration storeConfiguration =

hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java

@@ -670,8 +670,7 @@ private Pair<List<Path>, List<Path>> splitStoreFiles(final MasterProcedureEnv en
       String family = cfd.getNameAsString();
       Configuration trackerConfig = StoreFileTrackerFactory.
         mergeConfigurations(env.getMasterConfiguration(), htd, htd.getColumnFamily(cfd.getName()));
-      StoreFileTracker tracker = StoreFileTrackerFactory.create(trackerConfig, true,
-        family, regionFs);
+      StoreFileTracker tracker = StoreFileTrackerFactory.create(trackerConfig, family, regionFs);
       Collection<StoreFileInfo> sfis = tracker.load();
       if (sfis == null) {
         continue;

hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java

@@ -277,15 +277,17 @@ private boolean prepareCreate(final MasterProcedureEnv env) throws IOException {
       MasterProcedureUtil.checkGroupNotEmpty(rsGroupInfo, forWhom);
     }
 
+    // check for store file tracker configurations
+    StoreFileTrackerFactory.checkForCreateTable(env.getMasterConfiguration(), tableDescriptor);
+
     return true;
   }
 
   private void preCreate(final MasterProcedureEnv env)
       throws IOException, InterruptedException {
     if (!getTableName().isSystemTable()) {
-      ProcedureSyncWait.getMasterQuotaManager(env)
-        .checkNamespaceTableAndRegionQuota(
-          getTableName(), (newRegions != null ? newRegions.size() : 0));
+      ProcedureSyncWait.getMasterQuotaManager(env).checkNamespaceTableAndRegionQuota(getTableName(),
+        (newRegions != null ? newRegions.size() : 0));
     }
 
     TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableDescriptor);

hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java

@@ -38,6 +38,7 @@
 import org.apache.hadoop.hbase.master.MasterCoprocessorHost;
 import org.apache.hadoop.hbase.master.zksyncer.MetaLocationSyncer;
 import org.apache.hadoop.hbase.procedure2.ProcedureStateSerializer;
+import org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory;
 import org.apache.hadoop.hbase.replication.ReplicationException;
 import org.apache.hadoop.hbase.rsgroup.RSGroupInfo;
 import org.apache.hadoop.hbase.util.Bytes;
@@ -325,6 +326,10 @@ private void prepareModify(final MasterProcedureEnv env) throws IOException {
         modifiedTableDescriptor.getRegionServerGroup(), forWhom);
       MasterProcedureUtil.checkGroupNotEmpty(rsGroupInfo, forWhom);
     }
+
+    // check for store file tracker configurations
+    StoreFileTrackerFactory.checkForModifyTable(env.getMasterConfiguration(),
+      unmodifiedTableDescriptor, modifiedTableDescriptor);
   }
 
   /**

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

@@ -629,7 +629,7 @@ private void insertRegionFilesIntoStoreTracker(List<Path> allFiles, MasterProced
         Configuration config = StoreFileTrackerFactory.mergeConfigurations(conf, tblDesc,
           tblDesc.getColumnFamily(Bytes.toBytes(familyName)));
         return StoreFileTrackerFactory.
-          create(config, true, familyName, regionFs);
+          create(config, familyName, regionFs);
       });
       fileInfoMap.computeIfAbsent(familyName, l -> new ArrayList<>());
       List<StoreFileInfo> infos = fileInfoMap.get(familyName);

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

@@ -98,4 +98,12 @@ public void persistConfiguration(TableDescriptorBuilder builder) {
       builder.setValue(DST_IMPL, dst.getTrackerName());
     }
   }
+
+  static Class<? extends StoreFileTracker> getSrcTrackerClass(Configuration conf) {
+    return StoreFileTrackerFactory.getStoreFileTrackerClassForMigration(conf, SRC_IMPL);
+  }
+
+  static Class<? extends StoreFileTracker> getDstTrackerClass(Configuration conf) {
+    return StoreFileTrackerFactory.getStoreFileTrackerClassForMigration(conf, DST_IMPL);
+  }
 }

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

@@ -15,10 +15,12 @@
  */
 package org.apache.hadoop.hbase.regionserver.storefiletracker;
 
+import java.io.IOException;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.DoNotRetryIOException;
 import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
 import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
 import org.apache.hadoop.hbase.client.TableDescriptor;
@@ -111,44 +113,49 @@ public static StoreFileTracker create(Configuration conf, boolean isPrimaryRepli
    * 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,
+  public static StoreFileTracker create(Configuration conf, 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, isPrimaryReplica, ctx);
+    return StoreFileTrackerFactory.create(conf, true, ctx);
   }
 
   public static Configuration mergeConfigurations(Configuration global, TableDescriptor table,
     ColumnFamilyDescriptor family) {
     return StoreUtils.createStoreConfiguration(global, table, family);
   }
 
-  /**
-   * 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) {
+  static Class<? extends StoreFileTrackerBase>
+    getStoreFileTrackerClassForMigration(Configuration conf, String configName) {
     String trackerName =
       Preconditions.checkNotNull(conf.get(configName), "config %s is not set", configName);
-    Class<? extends StoreFileTrackerBase> tracker;
     try {
-      tracker =
-        Trackers.valueOf(trackerName.toUpperCase()).clazz.asSubclass(StoreFileTrackerBase.class);
+      return 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);
+        return Class.forName(trackerName).asSubclass(StoreFileTrackerBase.class);
       } catch (ClassNotFoundException cnfe) {
         throw new RuntimeException(cnfe);
       }
     }
+  }
+
+  /**
+   * 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) {
+    Class<? extends StoreFileTrackerBase> tracker =
+      getStoreFileTrackerClassForMigration(conf, configName);
     // 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");
+      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);
@@ -161,4 +168,142 @@ public static void persistTrackerConfig(Configuration conf, TableDescriptorBuild
     StoreFileTracker tracker = StoreFileTrackerFactory.create(conf, true, context);
     tracker.persistConfiguration(builder);
   }
+
+  // should not use MigrationStoreFileTracker for new family
+  private static void checkForNewFamily(Configuration conf, TableDescriptor table,
+    ColumnFamilyDescriptor family) throws IOException {
+    Configuration mergedConf = mergeConfigurations(conf, table, family);
+    Class<? extends StoreFileTracker> tracker = getTrackerClass(mergedConf);
+    if (MigrationStoreFileTracker.class.isAssignableFrom(tracker)) {
+      throw new DoNotRetryIOException(
+        "Should not use " + Trackers.MIGRATION + " as store file tracker for new family "
+          + family.getNameAsString() + " of table " + table.getTableName());
+    }
+  }
+
+  /**
+   * Pre check when creating a new table.
+   * <p/>
+   * For now, only make sure that we do not use {@link Trackers#MIGRATION} for newly created tables.
+   * @throws IOException when there are check errors, the upper layer should fail the
+   *           {@code CreateTableProcedure}.
+   */
+  public static void checkForCreateTable(Configuration conf, TableDescriptor table)
+    throws IOException {
+    for (ColumnFamilyDescriptor family : table.getColumnFamilies()) {
+      checkForNewFamily(conf, table, family);
+    }
+  }
+
+
+  /**
+   * Pre check when modifying a table.
+   * <p/>
+   * The basic idea is when you want to change the store file tracker implementation, you should use
+   * {@link Trackers#MIGRATION} first and then change to the destination store file tracker
+   * implementation.
+   * <p/>
+   * There are several rules:
+   * <ul>
+   * <li>For newly added family, you should not use {@link Trackers#MIGRATION}.</li>
+   * <li>For modifying a family:
+   * <ul>
+   * <li>If old tracker is {@link Trackers#MIGRATION}, then:
+   * <ul>
+   * <li>The new tracker is also {@link Trackers#MIGRATION}, then they must have the same src and
+   * dst tracker.</li>
+   * <li>The new tracker is not {@link Trackers#MIGRATION}, then the new tracker must be the dst
+   * tracker of the old tracker.</li>
+   * </ul>
+   * </li>
+   * <li>If the old tracker is not {@link Trackers#MIGRATION}, then:
+   * <ul>
+   * <li>If the new tracker is {@link Trackers#MIGRATION}, then the old tracker must be the src
+   * tracker of the new tracker.</li>
+   * <li>If the new tracker is not {@link Trackers#MIGRATION}, then the new tracker must be the same
+   * with old tracker.</li>
+   * </ul>
+   * </li>
+   * </ul>
+   * </li>
+   * </ul>
+   * @throws IOException when there are check errors, the upper layer should fail the
+   *           {@code ModifyTableProcedure}.
+   */
+  public static void checkForModifyTable(Configuration conf, TableDescriptor oldTable,
+    TableDescriptor newTable) throws IOException {
+    for (ColumnFamilyDescriptor newFamily : newTable.getColumnFamilies()) {
+      ColumnFamilyDescriptor oldFamily = oldTable.getColumnFamily(newFamily.getName());
+      if (oldFamily == null) {
+        checkForNewFamily(conf, newTable, newFamily);
+        continue;
+      }
+      Configuration oldConf = mergeConfigurations(conf, oldTable, oldFamily);
+      Configuration newConf = mergeConfigurations(conf, newTable, newFamily);
+
+      Class<? extends StoreFileTracker> oldTracker = getTrackerClass(oldConf);
+      Class<? extends StoreFileTracker> newTracker = getTrackerClass(newConf);
+
+      if (MigrationStoreFileTracker.class.isAssignableFrom(oldTracker)) {
+        Class<? extends StoreFileTracker> oldSrcTracker =
+          MigrationStoreFileTracker.getSrcTrackerClass(oldConf);
+        Class<? extends StoreFileTracker> oldDstTracker =
+          MigrationStoreFileTracker.getDstTrackerClass(oldConf);
+        if (oldTracker.equals(newTracker)) {
+          // confirm that we have the same src tracker and dst tracker
+          Class<? extends StoreFileTracker> newSrcTracker =
+            MigrationStoreFileTracker.getSrcTrackerClass(newConf);
+          if (!oldSrcTracker.equals(newSrcTracker)) {
+            throw new DoNotRetryIOException(
+              "The src tracker has been changed from " + getStoreFileTrackerName(oldSrcTracker)
+                + " to " + getStoreFileTrackerName(newSrcTracker) + " for family "
+                + newFamily.getNameAsString() + " of table " + newTable.getTableName());
+          }
+          Class<? extends StoreFileTracker> newDstTracker =
+            MigrationStoreFileTracker.getDstTrackerClass(newConf);
+          if (!oldDstTracker.equals(newDstTracker)) {
+            throw new DoNotRetryIOException(
+              "The dst tracker has been changed from " + getStoreFileTrackerName(oldDstTracker)
+                + " to " + getStoreFileTrackerName(newDstTracker) + " for family "
+                + newFamily.getNameAsString() + " of table " + newTable.getTableName());
+          }
+        } else {
+          // we can only change to the dst tracker
+          if (!newTracker.equals(oldDstTracker)) {
+            throw new DoNotRetryIOException(
+              "Should migrate tracker to " + getStoreFileTrackerName(oldDstTracker) + " but got "
+                + getStoreFileTrackerName(newTracker) + " for family " + newFamily.getNameAsString()
+                + " of table " + newTable.getTableName());
+          }
+        }
+      } else {
+        if (!oldTracker.equals(newTracker)) {
+          // can only change to MigrationStoreFileTracker and the src tracker should be the old
+          // tracker
+          if (!MigrationStoreFileTracker.class.isAssignableFrom(newTracker)) {
+            throw new DoNotRetryIOException("Should change to " + Trackers.MIGRATION
+              + " first when migrating from " + getStoreFileTrackerName(oldTracker) + " for family "
+              + newFamily.getNameAsString() + " of table " + newTable.getTableName());
+          }
+          Class<? extends StoreFileTracker> newSrcTracker =
+            MigrationStoreFileTracker.getSrcTrackerClass(newConf);
+          if (!oldTracker.equals(newSrcTracker)) {
+            throw new DoNotRetryIOException(
+              "Should use src tracker " + getStoreFileTrackerName(oldTracker) + " first but got "
+                + getStoreFileTrackerName(newSrcTracker) + " when migrating from "
+                + getStoreFileTrackerName(oldTracker) + " for family " + newFamily.getNameAsString()
+                + " of table " + newTable.getTableName());
+          }
+          Class<? extends StoreFileTracker> newDstTracker =
+            MigrationStoreFileTracker.getDstTrackerClass(newConf);
+          // the src and dst tracker should not be the same
+          if (newSrcTracker.equals(newDstTracker)) {
+            throw new DoNotRetryIOException("The src tracker and dst tracker are both "
+              + getStoreFileTrackerName(newSrcTracker) + " for family "
+              + newFamily.getNameAsString() + " of table " + newTable.getTableName());
+          }
+        }
+      }
+    }
+  }
 }