HBASE-26611 Changing SFT implementation on disabled table is dangerous

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

@@ -327,7 +327,7 @@ private void prepareModify(final MasterProcedureEnv env) throws IOException {
 
     // check for store file tracker configurations
     StoreFileTrackerValidationUtils.checkForModifyTable(env.getMasterConfiguration(),
-      unmodifiedTableDescriptor, modifiedTableDescriptor);
+      unmodifiedTableDescriptor, modifiedTableDescriptor, !isTableEnabled(env));
   }
 
   /**

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

@@ -20,6 +20,7 @@
 import java.io.IOException;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.DoNotRetryIOException;
+import org.apache.hadoop.hbase.TableNotEnabledException;
 import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor;
 import org.apache.hadoop.hbase.client.TableDescriptor;
 import org.apache.hadoop.hbase.regionserver.StoreUtils;
@@ -94,7 +95,7 @@ public static void checkForCreateTable(Configuration conf, TableDescriptor table
    *           {@code ModifyTableProcedure}.
    */
   public static void checkForModifyTable(Configuration conf, TableDescriptor oldTable,
-    TableDescriptor newTable) throws IOException {
+    TableDescriptor newTable, boolean isTableDisabled) throws IOException {
     for (ColumnFamilyDescriptor newFamily : newTable.getColumnFamilies()) {
       ColumnFamilyDescriptor oldFamily = oldTable.getColumnFamily(newFamily.getName());
       if (oldFamily == null) {
@@ -133,6 +134,16 @@ public static void checkForModifyTable(Configuration conf, TableDescriptor oldTa
               newFamily.getNameAsString() + " of table " + newTable.getTableName());
           }
         } else {
+          // do not allow changing from MIGRATION to its dst SFT implementation while the table is
+          // disabled. We need to open the HRegion to migrate the tracking information while the SFT
+          // implementation is MIGRATION, otherwise we may loss data. See HBASE-26611 for more
+          // details.
+          if (isTableDisabled) {
+            throw new TableNotEnabledException(
+              "Should not change store file tracker implementation from " +
+                StoreFileTrackerFactory.Trackers.MIGRATION.name() + " while table " +
+                newTable.getTableName() + " is disabled");
+          }
           // we can only change to the dst tracker
           if (!newTracker.equals(oldDstTracker)) {
             throw new DoNotRetryIOException("Should migrate tracker to " +
@@ -151,6 +162,9 @@ public static void checkForModifyTable(Configuration conf, TableDescriptor oldTa
                 StoreFileTrackerFactory.getStoreFileTrackerName(oldTracker) + " for family " +
                 newFamily.getNameAsString() + " of table " + newTable.getTableName());
           }
+          // here we do not check whether the table is disabled, as after changing to MIGRATION, we
+          // still rely on the src SFT implementation to actually load the store files, so there
+          // will be no data loss problem.
           Class<? extends StoreFileTracker> newSrcTracker =
             MigrationStoreFileTracker.getSrcTrackerClass(newConf);
           if (!oldTracker.equals(newSrcTracker)) {

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

@@ -19,13 +19,15 @@
 
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
 
 import java.io.IOException;
 import org.apache.hadoop.hbase.DoNotRetryIOException;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
 import org.apache.hadoop.hbase.HBaseTestingUtil;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.TableNameTestRule;
+import org.apache.hadoop.hbase.TableNotEnabledException;
 import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
 import org.apache.hadoop.hbase.client.Get;
 import org.apache.hadoop.hbase.client.Put;
@@ -195,6 +197,26 @@ public void testModifyError8() throws IOException {
     UTIL.getAdmin().modifyTable(newTd);
   }
 
+  @Test
+  public void testModifyError9() throws IOException {
+    TableDescriptor td = TableDescriptorBuilder.newBuilder(tableName.getTableName())
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.of("family")).build();
+    UTIL.getAdmin().createTable(td);
+    UTIL.getAdmin().disableTable(td.getTableName());
+    TableDescriptor newTd = TableDescriptorBuilder.newBuilder(td)
+      .setValue(StoreFileTrackerFactory.TRACKER_IMPL,
+        StoreFileTrackerFactory.Trackers.MIGRATION.name())
+      .setValue(MigrationStoreFileTracker.SRC_IMPL, StoreFileTrackerFactory.Trackers.DEFAULT.name())
+      .setValue(MigrationStoreFileTracker.DST_IMPL, StoreFileTrackerFactory.Trackers.FILE.name())
+      .build();
+    UTIL.getAdmin().modifyTable(newTd);
+    TableDescriptor newTd2 = TableDescriptorBuilder.newBuilder(td)
+      .setValue(StoreFileTrackerFactory.TRACKER_IMPL, StoreFileTrackerFactory.Trackers.FILE.name())
+      .build();
+    // changing from MIGRATION while table is disabled is not allowed
+    assertThrows(TableNotEnabledException.class, () -> UTIL.getAdmin().modifyTable(newTd2));
+  }
+
   private String getStoreFileName(TableName table, byte[] family) {
     return Iterables
       .getOnlyElement(Iterables.getOnlyElement(UTIL.getMiniHBaseCluster().getRegions(table))