[GitHub] [hudi] yihua commented on a change in pull request #4962: [HUDI-3355] Issue with out of order commits in the timeline when ingestion writers using SparkAllowUpdateStrategy

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

yihua commented on a change in pull request #4962:
URL: https://github.com/apache/hudi/pull/4962#discussion_r825395951



##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/utils/TransactionUtils.java
##########
@@ -80,11 +85,24 @@
       final Option<HoodieCommitMetadata> thisCommitMetadata,
       final HoodieWriteConfig config,
       Option<HoodieInstant> lastCompletedTxnOwnerInstant,
-      boolean reloadActiveTimeline) throws HoodieWriteConflictException {
+      boolean reloadActiveTimeline,
+      List<HoodieInstant> pendingClusteringInstants) throws HoodieWriteConflictException {
     if (config.getWriteConcurrencyMode().supportsOptimisticConcurrencyControl()) {
+      // deal with pendingClusteringInstants
+      // some clustering instants maybe finished during current write operation,
+      // we should check the conflict of those clustering operation
+      List<String> completeClusteringOperations = table.getMetaClient()

Review comment:
       Use `Set<String>` for `completeClusteringOperations` to achieve O(1) lookup with `contains`?

##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java
##########
@@ -429,6 +431,7 @@ protected void preWrite(String instantTime, WriteOperationType writeOperationTyp
       HoodieTableMetaClient metaClient) {
     setOperationType(writeOperationType);
     this.lastCompletedTxnAndMetadata = TransactionUtils.getLastCompletedTxnInstantAndMetadata(metaClient);
+    this.pendingReplaceRequestedInstants = TransactionUtils.getPendingReplaceRequestedInstants(metaClient);

Review comment:
       @xiarixiaoyao could you add a test case for the scenario described in HUDI-3355, which is resolved by the new logic in this PR?

##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/utils/TransactionUtils.java
##########
@@ -105,6 +123,16 @@
     return thisCommitMetadata;
   }
 
+  public static Option<HoodieCommitMetadata> resolveWriteConflictIfAny(

Review comment:
       I see `TransactionUtils::resolveWriteConflictIfAny` is also called in `BaseCommitActionExecutor::autoCommit`.  Does that place require looking at the pending clustering instants as well?

##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/utils/TransactionUtils.java
##########
@@ -137,4 +165,20 @@
       throw new HoodieIOException("Unable to read metadata for instant " + hoodieInstantOption.get(), io);
     }
   }
+
+  /**
+   * Get pending clustering instant.
+   * Notice:
+   *   we return .requested instant here.
+   *
+   * @param metaClient
+   * @return
+   */
+  public static List<HoodieInstant> getPendingReplaceRequestedInstants(HoodieTableMetaClient metaClient) {
+    return metaClient
+        .getActiveTimeline()
+        .filterPendingReplaceTimeline()
+        .getInstants()
+        .map(f -> HoodieTimeline.getReplaceCommitRequestedInstant(f.getTimestamp())).collect(Collectors.toList());

Review comment:
       I understand this addresses the completed replacecommit which was pending before the lastSuccessfulCommit looked by the conflict resolution.  Is there any other type of pending instants we need to consider as well, e.g., commits, deltacommits, compaction, etc.?  They could also be missed during conflict resolution like the pending clustering? 




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