voonhous commented on code in PR #19369:
URL: https://github.com/apache/hudi/pull/19369#discussion_r3802700818
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/marker/TimelineServerBasedWriteMarkers.java:
##########
@@ -168,7 +170,8 @@ public Option<StoragePath>
createWithEarlyConflictDetection(String partitionPath
* @param markerFileName Marker file name.
* @return {@code true} if successful; {@code false} otherwise.
*/
- private boolean executeCreateMarkerRequest(Map<String, String> paramsMap,
String partitionPath, String markerFileName) {
+ @VisibleForTesting
+ boolean executeCreateMarkerRequest(Map<String, String> paramsMap, String
partitionPath, String markerFileName) {
Review Comment:
`executeCreateMarkerRequest` hands back a bare boolean, and that boolean
carries three distinct outcomes: created, already exists, and
early-conflict-detected. Collapsing them here is what lets the new throw above
fire for a conflict, and the exception type matters.
`createWithEarlyConflictDetection` in this same file throws
`HoodieEarlyConflictDetectionException` for the identical `false`, and
`HoodieAppendHandle` keys off exactly that type:
```java
if (!config.getIgnoreWriteFailed() || ExceptionUtil.isCausedBy(e,
HoodieEarlyConflictDetectionException.class)) {
throw new HoodieException(e.getMessage(), e);
}
```
`hoodie.write.ignore.failed` defaults to `true`
(`HoodieWriteConfig.IGNORE_FAILED`), so a conflict that arrives as
`HoodieIOException` is swallowed into `writeStatus.markFailure` instead of
aborting the write. This is reachable: with server-side early conflict
detection enabled, `MarkerHandler#createMarker` runs detection for every create
on that marker dir, and `WriteMarkers` routes pending compaction and clustering
instants through the plain `create(..., false)` path even under OCC.
Concrete suggestion: return the outcome from the server rather than a
boolean, and map each one separately -- `Option.of` for created,
`Option.empty()` for already-exists when `checkIfExists` is set, and
`HoodieEarlyConflictDetectionException` for a detected conflict. If you would
rather keep the wire format as-is for now, at minimum do not convert a conflict
into `HoodieIOException`.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/marker/TimelineServerBasedWriteMarkers.java:
##########
@@ -137,7 +139,7 @@ protected Option<StoragePath> create(String partitionPath,
String fileName, IOTy
if (success) {
return Option.of(new
StoragePath(FSUtils.constructAbsolutePath(markerDirPath, partitionPath),
markerFileName));
} else {
- return Option.empty();
+ throw new HoodieIOException("[timeline-server-based] Failed to create
marker for partition " + partitionPath + ", fileName " + fileName + " with
IOType " + type);
Review Comment:
Digging into the server side, `false` has exactly two producers, and neither
one is a failed create:
- `MarkerDirState#processMarkerCreationRequests` ->
`future.setIsSuccessful(!exists)`, i.e. the marker already exists.
- the `catch (HoodieEarlyConflictDetectionException)` in that same method,
plus the equivalent in `MarkerHandler#createMarker` via
`finishCreateMarkerFuture`, i.e. early conflict detection fired.
A genuine failure never reaches the client as `false`. Transport errors
already become `HoodieRemoteException` inside `executeCreateMarkerRequest`, and
a marker flush failure throws `HoodieIOException` out of
`processMarkerCreationRequests` before the futures are completed (that leg has
since been fixed on master by #19368). So the message on this exception names a
condition that this branch cannot actually be in.
@nsivabalan on "a client can never ask to create a marker that already
exists" -- the log-marker path can, and the surrounding code is built to
tolerate it:
- `HoodieWriteHandle#getLogCreationCallback` returns
`createLogMarkerIfNotExists(...).isPresent()`, and all three consumers discard
that boolean (`HoodieLogFormatWriter#createNewFile`,
`HoodieNativeLogFormatWriter`, `HoodieNativeCDCFileWriter`). An empty Option
was designed as a tolerated no-op.
- `HoodieLogFormatWriter#getOutputStream` is an explicit collision-recovery
loop: `while (!created) { if (exists(logFile)) rollOver(); createNewFile(); }`
with `catch (FileAlreadyExistsException) { rollOver(); }`. `createNewFile()`
fires `preFileCreation`, i.e. creates the marker, *before*
`storage.create(path, false)`. A task that dies in that window leaves a marker
with no log file behind it; the retry recomputes the same rolled-over name and,
with this change, throws on that attempt and every later one, because the
marker persists in `.hoodie/.temp/<instant>` for the life of the instant. Two
writers racing the rollover hit the same thing: the loser used to recover
through `FileAlreadyExistsException -> rollOver`.
- The write token is not always distinct per attempt. `HoodieWriteHandle`
inherits `latestLogFile.getLogWriteToken()` for table version below 8,
`FSUtils.makeWriteToken` falls back to the constant
`HoodieLogFormat.DEFAULT_WRITE_TOKEN` ("0-0-0") when the task context is
unavailable, and `JavaTaskContextSupplier` returns 0/0/0 unconditionally.
- `RollbackHelperV1` uses `createIfNotExists(..., IOType.APPEND, ...)` for
rollback log appends; a retried rollback reuses the same rollback instant, so
the same marker dir and the same log file names.
Concrete suggestion: gate the throw on `checkIfExists`, so
`createIfNotExists` keeps returning `Option.empty()`. That matches
`DirectWriteMarkers.create`, which returns empty for already-exists and
reserves a throw for real I/O failure, and it matches the contract written on
the method being overridden -- `WriteMarkers#create(String, String, IOType,
boolean)`: "the marker path or empty option if already exists and
`checkIfExists` is true". The more complete fix is to have the server report
which of the three outcomes happened, since a bare boolean cannot express it.
--
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]
