hudi-agent commented on code in PR #19369:
URL: https://github.com/apache/hudi/pull/19369#discussion_r3653987463
##########
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:
🤖 This `create(...)` override ignores its `checkIfExists` param, so it now
throws on the `createIfNotExists` path too. The server returns `success=false`
both on a real failure and when the marker already exists, whereas
`DirectWriteMarkers.create()` returns `Option.empty()` for the already-exists
case. Callers that rely on that idempotency — `createLogMarkerIfNotExists` in
`HoodieWriteHandle`, and Flink's
`FlinkCreateHandle`/`FlinkMergeHandle.createMarkerFile` — would now get an
exception instead of an empty Option. Could we gate the throw on
`!checkIfExists` (or otherwise distinguish already-exists from failure) so the
two marker impls stay consistent? @nsivabalan
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/marker/TestTimelineServerBasedWriteMarkers.java:
##########
@@ -162,6 +168,54 @@ private void restartServerAndClient(int
numberOfSimulatedConnectionFailures,
}
}
+ @Test
+ public void testMarkerCreationFailure() throws IOException {
+ FileSystemViewStorageConfig.Builder builder =
FileSystemViewStorageConfig.newBuilder().withRemoteServerHost("localhost")
+ .withRemoteServerPort(timelineService.getServerPort())
+ .withRemoteTimelineClientTimeoutSecs(DEFAULT_READ_TIMEOUT_SECS);
+ MockTimelineServerBasedWriteMarkers timelineServerBasedWriteMarkers = new
MockTimelineServerBasedWriteMarkers(basePath, markerFolderPath.toString(),
"000", builder.build());
+ // this should succeed.
+ timelineServerBasedWriteMarkers.create("2020/06/01", "file1",
IOType.MERGE);
+
+ assertTrue(storage.exists(markerFolderPath));
+ assertTrue(writeMarkers.doesMarkerDirExist());
+
+ // lets fail the marker creation
+ timelineServerBasedWriteMarkers.failMarkerCreation = true;
+ try {
+ timelineServerBasedWriteMarkers.create("2020/06/01", "file2",
IOType.MERGE);
+ fail("Should not have reached here");
+ } catch (HoodieIOException ioe) {
+ assertTrue(ioe.getMessage().contains("[timeline-server-based] Failed to
create marker for partition"));
+ } finally {
+ if (timelineService != null) {
+ timelineService.close();
+ }
+ }
+ }
+
+ static class MockTimelineServerBasedWriteMarkers extends
TimelineServerBasedWriteMarkers {
+
+ boolean failMarkerCreation = false;
Review Comment:
🤖 nit: `failMarkerCreation` reads as an imperative verb rather than a
boolean predicate — could you rename it to something like
`shouldFailMarkerCreation` so that `if (!shouldFailMarkerCreation)` reads more
naturally?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
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:
🤖 One case worth double-checking here: `createIfNotExists` /
`createLogMarkerIfNotExists` route through this *same* overridden `create(...,
checkIfExists=true)`, and this timeline-server impl ignores `checkIfExists`. So
the server returning `false` for an already-existing marker
(`MarkerDirState#processMarkerCreationRequests` does
`future.setIsSuccessful(!exists)`) now throws for those callers too, where it
previously returned a benign `Option.empty()`.
A couple of real callers depend on that no-op rather than a fresh write
token: `HoodieWriteHandle#getLogCreationCallback` calls
`createLogMarkerIfNotExists(...).isPresent()` (so an existing APPEND log marker
is expected to yield empty, not an exception), and `RollbackHelperV1` uses
`createIfNotExists` for APPEND markers during rollback retries. Since genuine
I/O failures already surface as `HoodieRemoteException`, would it be safer to
scope the throw to the `checkIfExists == false` path (or have the server
distinguish already-exists from a real create failure) so the `*IfNotExists`
contract is preserved?
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/marker/TestTimelineServerBasedWriteMarkers.java:
##########
@@ -162,6 +168,54 @@ private void restartServerAndClient(int
numberOfSimulatedConnectionFailures,
}
}
+ @Test
+ public void testMarkerCreationFailure() throws IOException {
+ FileSystemViewStorageConfig.Builder builder =
FileSystemViewStorageConfig.newBuilder().withRemoteServerHost("localhost")
+ .withRemoteServerPort(timelineService.getServerPort())
+ .withRemoteTimelineClientTimeoutSecs(DEFAULT_READ_TIMEOUT_SECS);
+ MockTimelineServerBasedWriteMarkers timelineServerBasedWriteMarkers = new
MockTimelineServerBasedWriteMarkers(basePath, markerFolderPath.toString(),
"000", builder.build());
+ // this should succeed.
+ timelineServerBasedWriteMarkers.create("2020/06/01", "file1",
IOType.MERGE);
+
+ assertTrue(storage.exists(markerFolderPath));
+ assertTrue(writeMarkers.doesMarkerDirExist());
+
+ // lets fail the marker creation
+ timelineServerBasedWriteMarkers.failMarkerCreation = true;
+ try {
+ timelineServerBasedWriteMarkers.create("2020/06/01", "file2",
IOType.MERGE);
+ fail("Should not have reached here");
+ } catch (HoodieIOException ioe) {
+ assertTrue(ioe.getMessage().contains("[timeline-server-based] Failed to
create marker for partition"));
+ } finally {
+ if (timelineService != null) {
+ timelineService.close();
+ }
+ }
+ }
+
+ static class MockTimelineServerBasedWriteMarkers extends
TimelineServerBasedWriteMarkers {
+
+ boolean failMarkerCreation = false;
+
+ public MockTimelineServerBasedWriteMarkers(HoodieTable table, String
instantTime) {
+ super(table, instantTime);
+ }
+
+ MockTimelineServerBasedWriteMarkers(String basePath, String
markerFolderPath, String instantTime, FileSystemViewStorageConfig
fileSystemViewStorageConfig) {
+ super(basePath, markerFolderPath, instantTime,
fileSystemViewStorageConfig);
+ }
+
+ @Override
+ boolean executeCreateMarkerRequest(Map<String, String> paramsMap, String
partitionPath, String markerFileName) {
+ if (!failMarkerCreation) {
+ return super.executeCreateMarkerRequest(paramsMap, partitionPath,
markerFileName);
Review Comment:
🤖 nit: the `else` branch here is redundant since the `if` block always
returns — dropping it to a bare `return false;` after the closing brace would
be slightly cleaner.
<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]
