[GitHub] [hbase] Apache9 commented on a change in pull request #3721: HBASE-26326 CreateTableProcedure fails when FileBasedStoreFileTracker…

[GitBox <gi...@apache.org>]

Apache9 commented on a change in pull request #3721:
URL: https://github.com/apache/hbase/pull/3721#discussion_r724885509



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/FileBasedStoreFileTracker.java
##########
@@ -50,13 +50,15 @@
 @InterfaceAudience.Private
 class FileBasedStoreFileTracker extends StoreFileTrackerBase {
 
-  private final StoreFileListFile backedFile;
+  private StoreFileListFile backedFile;
 
   private final Map<String, StoreFileInfo> storefiles = new HashMap<>();
 
   public FileBasedStoreFileTracker(Configuration conf, boolean isPrimaryReplica, StoreContext ctx) {
     super(conf, isPrimaryReplica, ctx);
-    backedFile = new StoreFileListFile(ctx);
+    if (ctx != null && ctx.getFamilyStoreDirectoryPath() != null) {

Review comment:
       Just test whether ctx is null?
   And please add comments here to say that, when we want to add the store tracker configs to TableDescriptor, we will create store file tracker with a null StoreContext as there is no 'Store' yet, the implementation should take care of this.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -158,12 +166,12 @@ static StoreFileTrackerBase createForMigration(Configuration conf, String config
     return ReflectionUtils.newInstance(tracker, conf, isPrimaryReplica, ctx);
   }
 
-  public static void persistTrackerConfig(Configuration conf, TableDescriptorBuilder builder) {
+  public static void updateDescriptor(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);
+    StoreFileTracker tracker = StoreFileTrackerFactory.getInstance(conf, true, context);

Review comment:
       Here we should create a new Composite configration to merge the global conf and TableDescriptor?
   And please just pass a null StoreContext, and add comments to mention the reason here.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java
##########
@@ -291,7 +291,7 @@ private void preCreate(final MasterProcedureEnv env)
     }
 
     TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableDescriptor);
-    StoreFileTrackerFactory.persistTrackerConfig(env.getMasterConfiguration(), builder);
+    StoreFileTrackerFactory.updateDescriptor(env.getMasterConfiguration(), builder);

Review comment:
       Let's use updateWithTrackerConfigs?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/FileBasedStoreFileTracker.java
##########
@@ -50,13 +50,15 @@
 @InterfaceAudience.Private
 class FileBasedStoreFileTracker extends StoreFileTrackerBase {
 
-  private final StoreFileListFile backedFile;
+  private StoreFileListFile backedFile;

Review comment:
       It could still be final I think.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -100,13 +102,19 @@ static String getStoreFileTrackerName(Class<? extends StoreFileTracker> clazz) {
     }
   }
 
-  public static StoreFileTracker create(Configuration conf, boolean isPrimaryReplica,
+  private static StoreFileTracker getInstance(Configuration conf, boolean isPrimaryReplica,
     StoreContext ctx) {
     Class<? extends StoreFileTracker> tracker = getTrackerClass(conf);
     LOG.info("instantiating StoreFileTracker impl {}", tracker.getName());
     return ReflectionUtils.newInstance(tracker, conf, isPrimaryReplica, ctx);
   }
 
+  public static StoreFileTracker create(Configuration conf, boolean isPrimaryReplica,

Review comment:
       There is no difference between this method and the method above?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org