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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
