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 @@ public class MergeTableRegionsProcedure
       String family = hcd.getNameAsString();
       Configuration trackerConfig =
         StoreFileTrackerFactory.mergeConfigurations(env.getMasterConfiguration(), htd, hcd);
-      StoreFileTracker tracker = StoreFileTrackerFactory.create(trackerConfig, true,
+      StoreFileTracker tracker = StoreFileTrackerFactory.create(trackerConfig, family, regionFs);
-        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 @@ public class SplitTableRegionProcedure
       String family = cfd.getNameAsString();
       Configuration trackerConfig = StoreFileTrackerFactory.
         mergeConfigurations(env.getMasterConfiguration(), htd, htd.getColumnFamily(cfd.getName()));
-      StoreFileTracker tracker = StoreFileTrackerFactory.create(trackerConfig, true,
+      StoreFileTracker tracker = StoreFileTrackerFactory.create(trackerConfig, family, regionFs);
-        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 @@ public class CreateTableProcedure
       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)
+      ProcedureSyncWait.getMasterQuotaManager(env).checkNamespaceTableAndRegionQuota(getTableName(),
-        .checkNamespaceTableAndRegionQuota(
+        (newRegions != null ? newRegions.size() : 0));
-          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.client.TableDescriptor;
 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 @@ public class ModifyTableProcedure
         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 @@ public class HRegionFileSystem {
         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 @@ class MigrationStoreFileTracker extends StoreFileTrackerBase {
       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,13 +113,13 @@ public final class StoreFileTrackerFactory {
    * 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,
@@ -125,30 +127,35 @@ public final class StoreFileTrackerFactory {
     return StoreUtils.createStoreConfiguration(global, table, family);
   }
 
+  static Class<? extends StoreFileTrackerBase>
+    getStoreFileTrackerClassForMigration(Configuration conf, String configName) {
+    String trackerName =
+      Preconditions.checkNotNull(conf.get(configName), "config %s is not set", configName);
+    try {
+      return Trackers.valueOf(trackerName.toUpperCase()).clazz
+        .asSubclass(StoreFileTrackerBase.class);
+    } catch (IllegalArgumentException e) {
+      // Fall back to them specifying a class name
+      try {
+        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) {
-    String trackerName =
+    Class<? extends StoreFileTrackerBase> tracker =
-      Preconditions.checkNotNull(conf.get(configName), "config %s is not set", configName);
+      getStoreFileTrackerClassForMigration(conf, configName);
-    Class<? extends StoreFileTrackerBase> tracker;
-    try {
-      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 " +
+      throw new IllegalArgumentException("Should not specify " + configName + " as "
-        Trackers.MIGRATION + " because it can not be nested");
+        + 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 final class StoreFileTrackerFactory {
     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());
+          }
+        }
+      }
+    }
+  }
 }

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

@@ -0,0 +1,242 @@
+/**
+ * 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.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+
+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.client.ColumnFamilyDescriptorBuilder;
+import org.apache.hadoop.hbase.client.Get;
+import org.apache.hadoop.hbase.client.Put;
+import org.apache.hadoop.hbase.client.Table;
+import org.apache.hadoop.hbase.client.TableDescriptor;
+import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.apache.hadoop.hbase.testclassification.RegionServerTests;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Iterables;
+
+/**
+ * Test changing store file tracker implementation by altering table.
+ */
+@Category({ RegionServerTests.class, MediumTests.class })
+public class TestChangeStoreFileTracker {
+
+  @ClassRule
+  public static final HBaseClassTestRule CLASS_RULE =
+    HBaseClassTestRule.forClass(TestChangeStoreFileTracker.class);
+
+  private static final HBaseTestingUtil UTIL = new HBaseTestingUtil();
+
+  @Rule
+  public final TableNameTestRule tableName = new TableNameTestRule();
+
+  @BeforeClass
+  public static void setUp() throws Exception {
+    UTIL.startMiniCluster(1);
+  }
+
+  @AfterClass
+  public static void tearDown() throws IOException {
+    UTIL.shutdownMiniCluster();
+  }
+
+  @Test(expected = DoNotRetryIOException.class)
+  public void testCreateError() throws IOException {
+    TableDescriptor td = TableDescriptorBuilder.newBuilder(tableName.getTableName())
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.of("family"))
+      .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().createTable(td);
+  }
+
+  @Test(expected = DoNotRetryIOException.class)
+  public void testModifyError1() throws IOException {
+    TableDescriptor td = TableDescriptorBuilder.newBuilder(tableName.getTableName())
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.of("family")).build();
+    UTIL.getAdmin().createTable(td);
+    TableDescriptor newTd = TableDescriptorBuilder.newBuilder(td)
+      .setValue(StoreFileTrackerFactory.TRACKER_IMPL, StoreFileTrackerFactory.Trackers.FILE.name())
+      .build();
+    UTIL.getAdmin().modifyTable(newTd);
+  }
+
+  @Test(expected = DoNotRetryIOException.class)
+  public void testModifyError2() throws IOException {
+    TableDescriptor td = TableDescriptorBuilder.newBuilder(tableName.getTableName())
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.of("family")).build();
+    UTIL.getAdmin().createTable(td);
+    TableDescriptor newTd = TableDescriptorBuilder.newBuilder(td)
+      .setValue(StoreFileTrackerFactory.TRACKER_IMPL,
+        StoreFileTrackerFactory.Trackers.MIGRATION.name())
+      .setValue(MigrationStoreFileTracker.SRC_IMPL, StoreFileTrackerFactory.Trackers.FILE.name())
+      .setValue(MigrationStoreFileTracker.DST_IMPL, StoreFileTrackerFactory.Trackers.DEFAULT.name())
+      .build();
+    UTIL.getAdmin().modifyTable(newTd);
+  }
+
+  @Test(expected = DoNotRetryIOException.class)
+  public void testModifyError3() throws IOException {
+    TableDescriptor td = TableDescriptorBuilder.newBuilder(tableName.getTableName())
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.of("family")).build();
+    UTIL.getAdmin().createTable(td);
+    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.DEFAULT.name())
+      .build();
+    UTIL.getAdmin().modifyTable(newTd);
+  }
+
+  // return the TableDescriptor for creating table
+  private TableDescriptor createTableAndChangeToMigrationTracker() throws IOException {
+    TableDescriptor td = TableDescriptorBuilder.newBuilder(tableName.getTableName())
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.of("family")).build();
+    UTIL.getAdmin().createTable(td);
+    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);
+    return td;
+  }
+
+  @Test(expected = DoNotRetryIOException.class)
+  public void testModifyError4() throws IOException {
+    TableDescriptor td = createTableAndChangeToMigrationTracker();
+    TableDescriptor newTd = TableDescriptorBuilder.newBuilder(td)
+      .setValue(StoreFileTrackerFactory.TRACKER_IMPL,
+        StoreFileTrackerFactory.Trackers.MIGRATION.name())
+      .setValue(MigrationStoreFileTracker.SRC_IMPL, StoreFileTrackerFactory.Trackers.FILE.name())
+      .setValue(MigrationStoreFileTracker.DST_IMPL, StoreFileTrackerFactory.Trackers.DEFAULT.name())
+      .build();
+    UTIL.getAdmin().modifyTable(newTd);
+  }
+
+  @Test(expected = DoNotRetryIOException.class)
+  public void testModifyError5() throws IOException {
+    TableDescriptor td = createTableAndChangeToMigrationTracker();
+    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.DEFAULT.name())
+      .build();
+    UTIL.getAdmin().modifyTable(newTd);
+  }
+
+  @Test(expected = DoNotRetryIOException.class)
+  public void testModifyError6() throws IOException {
+    TableDescriptor td = createTableAndChangeToMigrationTracker();
+    TableDescriptor newTd =
+      TableDescriptorBuilder.newBuilder(td).setValue(StoreFileTrackerFactory.TRACKER_IMPL,
+        StoreFileTrackerFactory.Trackers.DEFAULT.name()).build();
+    UTIL.getAdmin().modifyTable(newTd);
+  }
+
+  @Test(expected = DoNotRetryIOException.class)
+  public void testModifyError7() throws IOException {
+    TableDescriptor td = TableDescriptorBuilder.newBuilder(tableName.getTableName())
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.of("family")).build();
+    UTIL.getAdmin().createTable(td);
+    TableDescriptor newTd = TableDescriptorBuilder.newBuilder(tableName.getTableName())
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.of("family"))
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.newBuilder(Bytes.toBytes("family1"))
+        .setConfiguration(StoreFileTrackerFactory.TRACKER_IMPL,
+          StoreFileTrackerFactory.Trackers.MIGRATION.name())
+        .build())
+      .build();
+    UTIL.getAdmin().modifyTable(newTd);
+  }
+
+  // actually a NPE as we do not specify the src and dst impl for migration store file tracker
+  @Test(expected = IOException.class)
+  public void testModifyError8() throws IOException {
+    TableDescriptor td = TableDescriptorBuilder.newBuilder(tableName.getTableName())
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.of("family")).build();
+    UTIL.getAdmin().createTable(td);
+    TableDescriptor newTd =
+      TableDescriptorBuilder.newBuilder(td).setValue(StoreFileTrackerFactory.TRACKER_IMPL,
+        StoreFileTrackerFactory.Trackers.MIGRATION.name()).build();
+    UTIL.getAdmin().modifyTable(newTd);
+  }
+
+  private String getStoreFileName(TableName table, byte[] family) {
+    return Iterables
+      .getOnlyElement(Iterables.getOnlyElement(UTIL.getMiniHBaseCluster().getRegions(table))
+        .getStore(family).getStorefiles())
+      .getPath().getName();
+  }
+
+  @Test
+  public void testModify() throws IOException {
+    TableName tn = tableName.getTableName();
+    byte[] row = Bytes.toBytes("row");
+    byte[] family = Bytes.toBytes("family");
+    byte[] qualifier = Bytes.toBytes("qualifier");
+    byte[] value = Bytes.toBytes("value");
+    TableDescriptor td = TableDescriptorBuilder.newBuilder(tn)
+      .setColumnFamily(ColumnFamilyDescriptorBuilder.of(family)).build();
+    UTIL.getAdmin().createTable(td);
+    try (Table table = UTIL.getConnection().getTable(tn)) {
+      table.put(new Put(row).addColumn(family, qualifier, value));
+    }
+    UTIL.flush(tn);
+    String fileName = getStoreFileName(tn, family);
+
+    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);
+    assertEquals(fileName, getStoreFileName(tn, family));
+    try (Table table = UTIL.getConnection().getTable(tn)) {
+      assertArrayEquals(value, table.get(new Get(row)).getValue(family, qualifier));
+    }
+
+    TableDescriptor newTd2 = TableDescriptorBuilder.newBuilder(td)
+      .setValue(StoreFileTrackerFactory.TRACKER_IMPL, StoreFileTrackerFactory.Trackers.FILE.name())
+      .build();
+    UTIL.getAdmin().modifyTable(newTd2);
+    assertEquals(fileName, getStoreFileName(tn, family));
+    try (Table table = UTIL.getConnection().getTable(tn)) {
+      assertArrayEquals(value, table.get(new Get(row)).getValue(family, qualifier));
+    }
+  }
+}