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

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

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTracker.java
##########
@@ -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);
+  TableDescriptor updateWithTrackerConfigs(TableDescriptorBuilder builder);

Review comment:
       Maybe `buildWithTrackerConfigs` if you're returning the `TableDescriptor` instead of the `TableDescriptorBuilder`. IMO, i'd just return the Builder back and let the caller `build()` when they're ready.

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCreateTableProcedure.java
##########
@@ -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());

Review comment:
       Does this need to be unset for other test methods in this class? Or, if tests are executed in parallel, maybe it would affects the other methods in this class? We set `forkCount` in surefire-plugin, but we don't set `parallel` (which I guess means we don't do parallel tests?). Genuinely not sure :)




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