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

… tracker (apache#3681)

Signed-off-by: Josh Elser <[email protected]>

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

@@ -93,4 +93,12 @@ static Class<? extends StoreFileTracker> getSrcTrackerClass(Configuration conf)
   static Class<? extends StoreFileTracker> getDstTrackerClass(Configuration conf) {
     return StoreFileTrackerFactory.getStoreFileTrackerClassForMigration(conf, DST_IMPL);
   }
+
+  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

@@ -311,4 +311,142 @@ public static void checkForModifyTable(Configuration conf, TableDescriptor oldTa
       }
     }
   }
+
+  // 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());
+          }
+        }
+      }
+    }
+  }
 }