HBASE-26326 CreateTableProcedure fails when FileBasedStoreFileTracker… (#3721)

Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Josh Elser <elserj@apache.org>

Author: wchevreuil

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) {

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

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<StoreFileInfo> 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<? extends StoreFileTracker> getSrcTrackerClass(Configuration conf) {
     return StoreFileTrackerFactory.getStoreFileTrackerClassForMigration(conf, SRC_IMPL);
   }

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,13 +76,13 @@ void replace(Collection<StoreFileInfo> compactedFiles, Collection<StoreFileInfo>
   StoreFileWriter createWriter(CreateStoreFileWriterParams params) throws IOException;
 
   /**
-   * Saves StoreFileTracker implementations specific configurations into the table descriptors.
+   * Adds StoreFileTracker implementations specific configurations into the table descriptor.
    * <p/>
    * 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.
    * <p/>
    * See HBASE-26246 for more details.
    * @param builder The table descriptor builder for the given table.
    */
-  void persistConfiguration(TableDescriptorBuilder builder);
+  TableDescriptorBuilder updateWithTrackerConfigs(TableDescriptorBuilder builder);
 }

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

@@ -25,14 +25,14 @@
 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;
 import org.apache.hadoop.hbase.io.hfile.CacheConfig;
 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<StoreFileInfo> 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() {

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

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<MasterProcedureEnv> 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<MasterProcedureEnv> procExec = getMasterProcedureExecutor();

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

@@ -40,7 +40,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<>());