From 4b493b226b451dc59ce3cb49551d8fa7844c6d35 Mon Sep 17 00:00:00 2001 From: Wellington Ramos Chevreuil Date: Wed, 13 Oct 2021 15:48:13 +0100 Subject: [PATCH] =?UTF-8?q?HBASE-26326=20CreateTableProcedure=20fails=20wh?= =?UTF-8?q?en=20FileBasedStoreFileTracker=E2=80=A6=20(#3721)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Duo Zhang Signed-off-by: Josh Elser --- .../procedure/CreateTableProcedure.java | 6 ++---- .../FileBasedStoreFileTracker.java | 9 ++++++++- .../MigrationStoreFileTracker.java | 12 +---------- .../storefiletracker/StoreFileTracker.java | 5 +++-- .../StoreFileTrackerBase.java | 9 ++++----- .../StoreFileTrackerFactory.java | 20 +++++++++++++------ .../procedure/TestCreateTableProcedure.java | 16 +++++++++++++++ .../TestStoreFileTracker.java | 2 +- 8 files changed, 49 insertions(+), 30 deletions(-) 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 80ed96a77f24..55e32126bdfb 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 @@ -32,7 +32,6 @@ import org.apache.hadoop.hbase.client.RegionInfo; import org.apache.hadoop.hbase.client.RegionReplicaUtil; import org.apache.hadoop.hbase.client.TableDescriptor; -import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.client.TableState; import org.apache.hadoop.hbase.master.MasterCoprocessorHost; import org.apache.hadoop.hbase.master.MasterFileSystem; @@ -270,9 +269,8 @@ private void preCreate(final MasterProcedureEnv env) (newRegions != null ? newRegions.size() : 0)); } - TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableDescriptor); - StoreFileTrackerFactory.persistTrackerConfig(env.getMasterConfiguration(), builder); - tableDescriptor = builder.build(); + tableDescriptor = StoreFileTrackerFactory.updateWithTrackerConfigs(env.getMasterConfiguration(), + tableDescriptor); final MasterCoprocessorHost cpHost = env.getMasterCoprocessorHost(); if (cpHost != null) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/FileBasedStoreFileTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/FileBasedStoreFileTracker.java index c370b87c1154..4da7911bdded 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/FileBasedStoreFileTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/FileBasedStoreFileTracker.java @@ -56,7 +56,14 @@ class FileBasedStoreFileTracker extends StoreFileTrackerBase { public FileBasedStoreFileTracker(Configuration conf, boolean isPrimaryReplica, StoreContext ctx) { super(conf, isPrimaryReplica, ctx); - backedFile = new StoreFileListFile(ctx); + //CreateTableProcedure needs to instantiate the configured SFT impl, in order to update table + //descriptors with the SFT impl specific configs. By the time this happens, the table has no + //regions nor stores yet, so it can't create a proper StoreContext. + if (ctx != null) { + backedFile = new StoreFileListFile(ctx); + } else { + backedFile = null; + } } @Override 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 1946d4bf7eef..230c1ec1b7a8 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 @@ -21,6 +21,7 @@ import java.util.Collection; import java.util.List; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.procedure2.util.StringUtils; import org.apache.hadoop.hbase.regionserver.StoreContext; @@ -88,17 +89,6 @@ void set(List files) { "Should not call this method on " + getClass().getSimpleName()); } - @Override - public void persistConfiguration(TableDescriptorBuilder builder) { - super.persistConfiguration(builder); - if (StringUtils.isEmpty(builder.getValue(SRC_IMPL))) { - builder.setValue(SRC_IMPL, src.getTrackerName()); - } - if (StringUtils.isEmpty(builder.getValue(DST_IMPL))) { - builder.setValue(DST_IMPL, dst.getTrackerName()); - } - } - static Class getSrcTrackerClass(Configuration conf) { return StoreFileTrackerFactory.getStoreFileTrackerClassForMigration(conf, SRC_IMPL); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTracker.java index 59fe7ef52f96..fd8f7c99092d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTracker.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTracker.java @@ -21,6 +21,7 @@ import java.util.Collection; import java.util.List; +import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.regionserver.CreateStoreFileWriterParams; import org.apache.hadoop.hbase.regionserver.StoreFileInfo; @@ -75,7 +76,7 @@ void replace(Collection compactedFiles, Collection StoreFileWriter createWriter(CreateStoreFileWriterParams params) throws IOException; /** - * Saves StoreFileTracker implementations specific configurations into the table descriptors. + * Adds StoreFileTracker implementations specific configurations into the table descriptor. *

* This is used to avoid accidentally data loss when changing the cluster level store file tracker * implementation, and also possible misconfiguration between master and region servers. @@ -83,5 +84,5 @@ void replace(Collection compactedFiles, Collection * See HBASE-26246 for more details. * @param builder The table descriptor builder for the given table. */ - void persistConfiguration(TableDescriptorBuilder builder); + TableDescriptorBuilder updateWithTrackerConfigs(TableDescriptorBuilder builder); } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java index a786add49b21..edbaace40c26 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java @@ -25,6 +25,7 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; +import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; import org.apache.hadoop.hbase.io.compress.Compression; import org.apache.hadoop.hbase.io.crypto.Encryption; @@ -32,7 +33,6 @@ import org.apache.hadoop.hbase.io.hfile.HFile; import org.apache.hadoop.hbase.io.hfile.HFileContext; import org.apache.hadoop.hbase.io.hfile.HFileContextBuilder; -import org.apache.hadoop.hbase.procedure2.util.StringUtils; import org.apache.hadoop.hbase.regionserver.CreateStoreFileWriterParams; import org.apache.hadoop.hbase.regionserver.StoreContext; import org.apache.hadoop.hbase.regionserver.StoreFileInfo; @@ -83,10 +83,9 @@ public final void replace(Collection compactedFiles, } @Override - public void persistConfiguration(TableDescriptorBuilder builder) { - if (StringUtils.isEmpty(builder.getValue(TRACKER_IMPL))) { - builder.setValue(TRACKER_IMPL, getTrackerName()); - } + public TableDescriptorBuilder updateWithTrackerConfigs(TableDescriptorBuilder builder) { + builder.setValue(TRACKER_IMPL, getTrackerName()); + return builder; } protected final String getTrackerName() { 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 b586027f8333..1c683ae3de62 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 @@ -24,8 +24,10 @@ import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.client.TableDescriptorBuilder; +import org.apache.hadoop.hbase.procedure2.util.StringUtils; import org.apache.hadoop.hbase.regionserver.HRegionFileSystem; import org.apache.hadoop.hbase.regionserver.StoreContext; + import org.apache.hadoop.hbase.regionserver.StoreUtils; import org.apache.hadoop.hbase.util.ReflectionUtils; import org.apache.yetus.audience.InterfaceAudience; @@ -158,12 +160,18 @@ static StoreFileTrackerBase createForMigration(Configuration conf, String config return ReflectionUtils.newInstance(tracker, conf, isPrimaryReplica, ctx); } - public static void persistTrackerConfig(Configuration conf, TableDescriptorBuilder builder) { - TableDescriptor tableDescriptor = builder.build(); - ColumnFamilyDescriptor cfDesc = tableDescriptor.getColumnFamilies()[0]; - StoreContext context = StoreContext.getBuilder().withColumnFamilyDescriptor(cfDesc).build(); - StoreFileTracker tracker = StoreFileTrackerFactory.create(conf, true, context); - tracker.persistConfiguration(builder); + public static TableDescriptor updateWithTrackerConfigs(Configuration conf, + TableDescriptor descriptor) { + //CreateTableProcedure needs to instantiate the configured SFT impl, in order to update table + //descriptors with the SFT impl specific configs. By the time this happens, the table has no + //regions nor stores yet, so it can't create a proper StoreContext. + if (StringUtils.isEmpty(descriptor.getValue(TRACKER_IMPL))) { + StoreFileTracker tracker = + StoreFileTrackerFactory.create(conf, true, null); + TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(descriptor); + return tracker.updateWithTrackerConfigs(builder).build(); + } + return descriptor; } // should not use MigrationStoreFileTracker for new family diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCreateTableProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCreateTableProcedure.java index f432c8060d3d..51ea9f58248a 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCreateTableProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCreateTableProcedure.java @@ -39,6 +39,7 @@ import org.apache.hadoop.hbase.procedure2.Procedure; import org.apache.hadoop.hbase.procedure2.ProcedureExecutor; import org.apache.hadoop.hbase.procedure2.ProcedureTestingUtility; +import org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory; import org.apache.hadoop.hbase.regionserver.storefiletracker.TestStoreFileTracker; import org.apache.hadoop.hbase.testclassification.MasterTests; import org.apache.hadoop.hbase.testclassification.MediumTests; @@ -105,6 +106,21 @@ public void testCreateWithTrackImpl() throws Exception { assertEquals(trackerName, htd.getValue(TRACKER_IMPL)); } + @Test + public void testCreateWithFileBasedStoreTrackerImpl() throws Exception { + ProcedureExecutor procExec = getMasterProcedureExecutor(); + procExec.getEnvironment().getMasterConfiguration().set(StoreFileTrackerFactory.TRACKER_IMPL, + StoreFileTrackerFactory.Trackers.FILE.name()); + final TableName tableName = TableName.valueOf(name.getMethodName()); + TableDescriptor htd = MasterProcedureTestingUtility.createHTD(tableName, F1); + RegionInfo[] regions = ModifyRegionUtils.createRegionInfos(htd, null); + long procId = ProcedureTestingUtility.submitAndWait(procExec, + new CreateTableProcedure(procExec.getEnvironment(), htd, regions)); + ProcedureTestingUtility.assertProcNotFailed(procExec.getResult(procId)); + htd = getMaster().getTableDescriptors().get(tableName); + assertEquals(StoreFileTrackerFactory.Trackers.FILE.name(), htd.getValue(TRACKER_IMPL)); + } + @Test public void testCreateWithoutColumnFamily() throws Exception { final ProcedureExecutor procExec = getMasterProcedureExecutor(); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTracker.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTracker.java index 80d0ec6ebd9e..4a471baa8689 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTracker.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/storefiletracker/TestStoreFileTracker.java @@ -37,7 +37,7 @@ public class TestStoreFileTracker extends DefaultStoreFileTracker { public TestStoreFileTracker(Configuration conf, boolean isPrimaryReplica, StoreContext ctx) { super(conf, isPrimaryReplica, ctx); - if (ctx.getRegionFileSystem() != null) { + if (ctx != null && ctx.getRegionFileSystem() != null) { this.storeId = ctx.getRegionInfo().getEncodedName() + "-" + ctx.getFamily().getNameAsString(); LOG.info("created storeId: {}", storeId); trackedFiles.computeIfAbsent(storeId, v -> new ArrayList<>());