fix(timeline-service): fail marker creation when create request fails by nsivabalan · Pull Request #19369 · apache/hudi

nsivabalan · 2026-07-23T23:30:22Z

Change Logs

TimelineServerBasedWriteMarkers#create silently returned Option.empty() when the timeline server's marker creation request failed, instead of raising an error. Callers of WriteMarkers#create do not check the returned Option for emptiness, so a failed marker creation was treated as if it succeeded, allowing a write to proceed and create a data file without a durable marker for it. Marker-based rollback of a failed write would then miss such files.

Changes:

TimelineServerBasedWriteMarkers#create now throws HoodieIOException when the marker creation request fails, instead of returning Option.empty(), so the failure surfaces to the writer.
Relaxes executeCreateMarkerRequest's visibility to package-private so it can be overridden in tests to simulate a failure.
Adds testMarkerCreationFailure to TestTimelineServerBasedWriteMarkers, asserting the exception is thrown with the expected message on a simulated failure.

Impact

No public API change. Writers using timeline-server-based markers now get an exception (instead of a silent no-op) when a marker creation request fails, so a data file is not written without a durable marker.

Risk level

low. New unit test covers the failure path of marker creation.

Documentation Update

none

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

TimelineServerBasedWriteMarkers#create silently returned Option.empty() when the timeline server's marker creation request failed, instead of raising an error. Callers of WriteMarkers#create do not check the returned Option for emptiness, so a failed marker creation was treated as if it succeeded, allowing a write to proceed without a durable marker for the data file it wrote. Throw HoodieIOException when the marker creation request fails, so the failure surfaces to the writer instead of being silently swallowed. Also relaxes executeCreateMarkerRequest's visibility to package-private so it can be overridden in tests to simulate a failure.

hudi-bot · 2026-07-24T01:17:18Z

CI report:

a9fbee2 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

wombatu-kun · 2026-07-24T05:08:08Z

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


A false result here does not mean the create failed: the server returns false when the marker already exists (MarkerDirState sets isSuccessful to !exists), and a genuine I/O failure already surfaces as HoodieRemoteException, not as false. So this throw fires on an already-existing marker, which createIfNotExists/createLogMarkerIfNotExists treat as a harmless Option.empty no-op - is turning already-exists into a hard failure intended, or should the throw be limited to a real create failure?

hey @wombatu-kun : thanks for the note.
none of our code path should try to create an already existing marker. every file is immutable including log files. and task retries will use a diff write token. so, a client can never ask for to create a marker that already exists.

let me know if I am missing any case here

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

wombatu-kun · 2026-07-24T05:08:08Z

+      fail("Should not have reached here");
+    } catch (HoodieIOException ioe) {
+      assertTrue(ioe.getMessage().contains("[timeline-server-based] Failed to create marker for partition"));
+    } finally {


@AfterEach cleanup() already closes timelineService, so closing it again in this finally double-closes the service. Drop the finally - the try only needs to wrap the failing create.

wombatu-kun · 2026-07-24T05:08:08Z

+
+    boolean failMarkerCreation = false;
+
+    public MockTimelineServerBasedWriteMarkers(HoodieTable table, String instantTime) {


This MockTimelineServerBasedWriteMarkers(HoodieTable, String) constructor is unused - the test builds the mock through the (String, String, String, FileSystemViewStorageConfig) constructor. Remove it.

wombatu-kun · 2026-07-24T05:08:08Z

+      if (!failMarkerCreation) {
+        return super.executeCreateMarkerRequest(paramsMap, partitionPath, markerFileName);
+      } else {
+        return false;


Returning false here mirrors a create failure, but the real server also returns false when the marker already exists (MarkerDirState: !exists), so the test cannot tell a genuine failure from a benign already-exists and never covers createIfNotExists on an existing marker. Add a case that creates the same marker twice to lock down the intended behavior.

hudi-agent

⚠️ 🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Thanks for working on this! The PR closes a real gap — a failed timeline-server marker creation was silently swallowed as Option.empty(), letting a write proceed without a durable marker. Converting that into a HoodieIOException is the right direction. The main thing worth double-checking (in the inline comment) is that the throw now also fires on the createIfNotExists path, where a "marker already exists" result is expected and tolerated. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. A couple of minor naming and simplification suggestions in the test additions.

hudi-agent · 2026-07-27T01:56:21Z

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


🤖 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

_{⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.}

hudi-agent · 2026-07-27T01:56:21Z

+
+  static class MockTimelineServerBasedWriteMarkers extends TimelineServerBasedWriteMarkers {
+
+    boolean failMarkerCreation = false;


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

_{⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.}

hudi-agent · 2026-07-27T01:56:21Z

+    @Override
+    boolean executeCreateMarkerRequest(Map<String, String> paramsMap, String partitionPath, String markerFileName) {
+      if (!failMarkerCreation) {
+        return super.executeCreateMarkerRequest(paramsMap, partitionPath, markerFileName);


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

_{⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.}

hudi-agent · 2026-07-27T01:56:26Z

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


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

github-actions Bot added the size:S PR with lines of changes in (10, 100] label Jul 23, 2026

wombatu-kun reviewed Jul 24, 2026

View reviewed changes

hudi-agent reviewed Jul 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(timeline-service): fail marker creation when create request fails - #19369

fix(timeline-service): fail marker creation when create request fails#19369
nsivabalan wants to merge 1 commit into
apache:masterfrom
nsivabalan:fix-timeline-server-marker-creation-failure

nsivabalan commented Jul 23, 2026

Uh oh!

hudi-bot commented Jul 24, 2026

Uh oh!

wombatu-kun Jul 24, 2026

Uh oh!

nsivabalan Jul 24, 2026

Uh oh!

hudi-agent Jul 27, 2026

Uh oh!

wombatu-kun Jul 24, 2026

Uh oh!

wombatu-kun Jul 24, 2026

Uh oh!

wombatu-kun Jul 24, 2026

Uh oh!

hudi-agent left a comment

Uh oh!

hudi-agent Jul 27, 2026

Uh oh!

hudi-agent Jul 27, 2026

Uh oh!

hudi-agent Jul 27, 2026

Uh oh!

hudi-agent Jul 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		boolean failMarkerCreation = false;

		public MockTimelineServerBasedWriteMarkers(HoodieTable table, String instantTime) {


		static class MockTimelineServerBasedWriteMarkers extends TimelineServerBasedWriteMarkers {

		boolean failMarkerCreation = false;

Uh oh!

Conversation

nsivabalan commented Jul 23, 2026

Change Logs

Impact

Risk level

Documentation Update

Contributor's checklist

Uh oh!

hudi-bot commented Jul 24, 2026

CI report:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hudi-agent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants