From a816204793b1050cecf9fe8af0172c176c4511e7 Mon Sep 17 00:00:00 2001 From: Duo Zhang Date: Tue, 15 Mar 2022 20:13:46 +0800 Subject: [PATCH] HBASE-26611 Changing SFT implementation on disabled table is dangerous (#4082) Signed-off-by: Xiaolin Ha --- .../procedure/ModifyTableProcedure.java | 2 +- .../StoreFileTrackerValidationUtils.java | 16 +++++++++++++- .../TestChangeStoreFileTracker.java | 22 +++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) 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 9237f39d809..5019efe7f31 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 @@ -327,7 +327,7 @@ public class ModifyTableProcedure // check for store file tracker configurations StoreFileTrackerValidationUtils.checkForModifyTable(env.getMasterConfiguration(), - unmodifiedTableDescriptor, modifiedTableDescriptor); + unmodifiedTableDescriptor, modifiedTableDescriptor, !isTableEnabled(env)); } /** diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerValidationUtils.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerValidationUtils.java index e6f6e854c89..38040bc4f00 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerValidationUtils.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerValidationUtils.java @@ -20,6 +20,7 @@ package org.apache.hadoop.hbase.regionserver.storefiletracker; 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 final class StoreFileTrackerValidationUtils { * {@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 final class StoreFileTrackerValidationUtils { 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 final class StoreFileTrackerValidationUtils { 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 newSrcTracker = MigrationStoreFileTracker.getSrcTrackerClass(newConf); if (!oldTracker.equals(newSrcTracker)) { 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 index 70f62c02ed2..afce4ed4e63 100644 --- 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 @@ -19,6 +19,7 @@ package org.apache.hadoop.hbase.regionserver.storefiletracker; 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; @@ -26,6 +27,7 @@ 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 class TestChangeStoreFileTracker { 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))