[GitHub] [hudi] codope commented on a change in pull request #3947: [HUDI-2712] Fixing list based rollback of partially failed commit which has new partitions

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

codope commented on a change in pull request #3947:
URL: https://github.com/apache/hudi/pull/3947#discussion_r748382918



##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/rollback/RollbackUtils.java
##########
@@ -127,7 +129,7 @@ static HoodieRollbackStat mergeRollbackStat(HoodieRollbackStat stat1, HoodieRoll
   public static List<ListingBasedRollbackRequest> generateRollbackRequestsUsingFileListingMOR(HoodieInstant instantToRollback, HoodieTable table, HoodieEngineContext context) throws IOException {
     String commit = instantToRollback.getTimestamp();
     HoodieWriteConfig config = table.getConfig();
-    List<String> partitions = FSUtils.getAllPartitionPaths(context, config.getMetadataConfig(), table.getMetaClient().getBasePath());
+    List<String> partitions = FSUtils.getAllPartitionPaths(context, HoodieMetadataConfig.newBuilder().enable(false).build(), table.getMetaClient().getBasePath());

Review comment:
       This comment is not specific to the patch. Something to consider:
   Since there are many places where some metadata configs are hardcoded, would it be better to extract them to constants? For e.g. in this case we can introduce ROLLBACK_METADATA_ENABLE_DEFAULT = false and reuse the constant wherever needed. Add a small comment wherever we declare the constant why the default value makes sense. This would help to keep code maintainable and readable. 

##########
File path: hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/client/functional/TestHoodieBackedMetadata.java
##########
@@ -1093,8 +1150,9 @@ public void testErrorCases() throws Exception {
       newCommitTime = HoodieActiveTimeline.createNewInstantTime();
       client.startCommitWithTime(newCommitTime);
       records = dataGen.generateInserts(newCommitTime, 5);
-      writeStatuses = client.bulkInsert(jsc.parallelize(records, 1), newCommitTime).collect();
+      writeStatuses = client.insert(jsc.parallelize(records, 1), newCommitTime).collect();

Review comment:
       Why not keep bulkInsert?




-- 
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: commits-unsubscribe@hudi.apache.org

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