hudi-agent commented on code in PR #18119:
URL: https://github.com/apache/hudi/pull/18119#discussion_r3584687426
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/SimpleConcurrentFileWritesConflictResolutionStrategy.java:
##########
@@ -200,8 +200,66 @@ public Option<HoodieCommitMetadata>
resolveConflict(HoodieTable table,
return thisOperation.getCommitMetadataOption();
}
// just abort the current write if conflicts are found (failed for
rollback conflicts).
- throw new HoodieWriteConflictException(new
ConcurrentModificationException("Cannot resolve conflicts for overlapping
writes between first operation = " + thisOperation
- + ", second operation = " + otherOperation));
+ throw new HoodieWriteConflictException(new
ConcurrentModificationException(buildConflictErrorMessage(thisOperation,
otherOperation)));
+ }
+
+ /**
+ * Builds a detailed error message for write conflicts based on the
operation types involved.
+ */
+ private String buildConflictErrorMessage(ConcurrentOperation thisOperation,
ConcurrentOperation otherOperation) {
+ boolean thisIsTableService =
WriteOperationType.isTableService(thisOperation.getOperationType());
+ boolean otherIsTableService =
WriteOperationType.isTableService(otherOperation.getOperationType());
+ String thisOperationDescription =
formatOperationDescription(thisOperation);
+ String otherOperationDescription =
formatOperationDescription(otherOperation);
+ // If either operation is a table service, provide specific retry guidance
+ if (thisIsTableService || otherIsTableService) {
+ ConcurrentOperation tableServiceOperation = thisIsTableService ?
thisOperation : otherOperation;
Review Comment:
🤖 When both operations are table services (e.g. compaction conflicting with
clustering, which nsivabalan noted upthread is a real case), this arbitrarily
picks `thisOperation` as "the table service" and frames the other table service
as the "regular" write to retry — so the message can read like "Clustering ...
has overlapping file groups with Compaction ... retry the write after
clustering completes", which misrepresents which side the user controls. Might
be worth handling the both-are-table-services case explicitly.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/model/WriteOperationType.java:
##########
@@ -188,6 +188,17 @@ public static boolean
isCompactionOrClustering(WriteOperationType operationType)
return operationType == COMPACT || operationType == CLUSTER;
}
+ /**
+ * Checks if the given operation type is a table service operation.
+ * Table service operations include compaction, clustering, log compaction,
and indexing.
+ */
+ public static boolean isTableService(WriteOperationType operationType) {
+ return operationType == COMPACT
Review Comment:
🤖 nit: could you delegate to `isCompactionOrClustering` here instead of
repeating `COMPACT || CLUSTER`? Something like `return
isCompactionOrClustering(operationType) || operationType == LOG_COMPACT ||
operationType == INDEX;` — that way if a new compaction/clustering variant is
ever added to `isCompactionOrClustering`, this method stays in sync
automatically.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/transaction/SimpleConcurrentFileWritesConflictResolutionStrategy.java:
##########
@@ -200,8 +200,66 @@ public Option<HoodieCommitMetadata>
resolveConflict(HoodieTable table,
return thisOperation.getCommitMetadataOption();
}
// just abort the current write if conflicts are found (failed for
rollback conflicts).
- throw new HoodieWriteConflictException(new
ConcurrentModificationException("Cannot resolve conflicts for overlapping
writes between first operation = " + thisOperation
- + ", second operation = " + otherOperation));
+ throw new HoodieWriteConflictException(new
ConcurrentModificationException(buildConflictErrorMessage(thisOperation,
otherOperation)));
+ }
+
+ /**
+ * Builds a detailed error message for write conflicts based on the
operation types involved.
+ */
+ private String buildConflictErrorMessage(ConcurrentOperation thisOperation,
ConcurrentOperation otherOperation) {
+ boolean thisIsTableService =
WriteOperationType.isTableService(thisOperation.getOperationType());
+ boolean otherIsTableService =
WriteOperationType.isTableService(otherOperation.getOperationType());
+ String thisOperationDescription =
formatOperationDescription(thisOperation);
+ String otherOperationDescription =
formatOperationDescription(otherOperation);
+ // If either operation is a table service, provide specific retry guidance
+ if (thisIsTableService || otherIsTableService) {
+ ConcurrentOperation tableServiceOperation = thisIsTableService ?
thisOperation : otherOperation;
+ String tableServiceDescription = thisIsTableService ?
thisOperationDescription : otherOperationDescription;
+ String regularOperationDescription = thisIsTableService ?
otherOperationDescription : thisOperationDescription;
+ String serviceType =
getTableServiceDisplayName(tableServiceOperation.getOperationType());
+ return String.format(
+ "Cannot resolve conflicts for overlapping writes. %s is currently
running and has overlapping file groups with %s. "
Review Comment:
🤖 When the conflicting table service has already completed (e.g. a finished
clustering/compaction whose replaced file groups now overlap), this still says
the service "is currently running" and asks the user to "retry ... after the
[service] completes". That's contradictory for a COMPLETED instant — and the
new `testErrorMessageForConflictWithCompletedClustering` test actually asserts
state=COMPLETED alongside this "is currently running" wording. Could we key the
phrasing off `getInstantActionState()` so a completed service produces accurate
guidance (the write likely needs re-planning against the new file groups, not
just a retry-after-completion)?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
