From 5ff0f98a5367326753bc0e06d8726b66d109238c Mon Sep 17 00:00:00 2001 From: Duo Zhang Date: Wed, 15 Sep 2021 23:00:03 +0800 Subject: [PATCH] HBASE-26264 Add more checks to prevent misconfiguration on store file tracker (#3681) Signed-off-by: Josh Elser --- .../MergeTableRegionsProcedure.java | 3 +- .../assignment/SplitTableRegionProcedure.java | 3 +- .../procedure/CreateTableProcedure.java | 8 +- .../procedure/ModifyTableProcedure.java | 5 + .../hbase/regionserver/HRegionFileSystem.java | 2 +- .../MigrationStoreFileTracker.java | 8 + .../StoreFileTrackerFactory.java | 173 ++++++++++++- .../TestChangeStoreFileTracker.java | 242 ++++++++++++++++++ 8 files changed, 422 insertions(+), 22 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestChangeStoreFileTracker.java diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java index e6bbe445d077..e9051dab0913 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java @@ -614,8 +614,7 @@ private List 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 storeFiles = tracker.load(); if (storeFiles != null && storeFiles.size() > 0) { final Configuration storeConfiguration = diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java index ff16dc5514b8..aa0c938fa21f 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java @@ -670,8 +670,7 @@ private Pair, List> 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 sfis = tracker.load(); if (sfis == null) { continue; diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java index dccea5512c1f..ee8e51ffe86b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java +++ b/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); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java index 247dd9c202f4..1640644328f0 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java +++ b/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); } /** diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionFileSystem.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionFileSystem.java index aa0ee273ea9e..e78d8ad8aa1e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionFileSystem.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionFileSystem.java @@ -629,7 +629,7 @@ private void insertRegionFilesIntoStoreTracker(List 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 infos = fileInfoMap.get(familyName); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java index 3eeef9000576..1946d4bf7eef 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java +++ b/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 getSrcTrackerClass(Configuration conf) { + return StoreFileTrackerFactory.getStoreFileTrackerClassForMigration(conf, SRC_IMPL); + } + + static Class getDstTrackerClass(Configuration conf) { + return StoreFileTrackerFactory.getStoreFileTrackerClassForMigration(conf, DST_IMPL); + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java index 9be19ec15ed8..90704fe528a0 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java +++ b/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 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, @@ -125,30 +127,35 @@ public static Configuration mergeConfigurations(Configuration global, TableDescr 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 + getStoreFileTrackerClassForMigration(Configuration conf, String configName) { String trackerName = Preconditions.checkNotNull(conf.get(configName), "config %s is not set", configName); - Class 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 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 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. + *

+ * 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. + *

+ * 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. + *

+ * There are several rules: + *

    + *
  • For newly added family, you should not use {@link Trackers#MIGRATION}.
    • + *
  • For modifying a family: + *
      + *
    • If old tracker is {@link Trackers#MIGRATION}, then: + *
        + *
      • The new tracker is also {@link Trackers#MIGRATION}, then they must have the same src and + * dst tracker.
        • + *
      • The new tracker is not {@link Trackers#MIGRATION}, then the new tracker must be the dst + * tracker of the old tracker.
        • + *
      + *
      • + *
    • If the old tracker is not {@link Trackers#MIGRATION}, then: + *
        + *
      • If the new tracker is {@link Trackers#MIGRATION}, then the old tracker must be the src + * tracker of the new tracker.
        • + *
      • If the new tracker is not {@link Trackers#MIGRATION}, then the new tracker must be the same + * with old tracker.
        • + *
      + *
      • + *
    + *
    • + *
+ * @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 oldTracker = getTrackerClass(oldConf); + Class newTracker = getTrackerClass(newConf); + + if (MigrationStoreFileTracker.class.isAssignableFrom(oldTracker)) { + Class oldSrcTracker = + MigrationStoreFileTracker.getSrcTrackerClass(oldConf); + Class oldDstTracker = + MigrationStoreFileTracker.getDstTrackerClass(oldConf); + if (oldTracker.equals(newTracker)) { + // confirm that we have the same src tracker and dst tracker + Class 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 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 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 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()); + } + } + } + } + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestChangeStoreFileTracker.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestChangeStoreFileTracker.java new file mode 100644 index 000000000000..70f62c02ed28 --- /dev/null +++ b/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)); + } + } +}