vamsikarnika opened a new pull request, #19867:
URL: https://github.com/apache/hudi/pull/19867
### Describe the issue this Pull Request addresses
A commit that has already been made durable can be reported back to the
caller as a **failed** write,
purely because of heartbeat file cleanup.
`stop()` cancels the heartbeat scheduler with `shutdownNow()` and
immediately deletes the heartbeat
file, without waiting for a refresh that is already in flight. The refresh
recreates the file, and on
storage that enforces preconditions it also changes the object generation,
so the generation-matched
delete is rejected. Observed in production on GCS as `412 conditionNotMet`,
retried four times by the
connector and then given up on:
```
Precondition not met while deleting '.../.hoodie/.heartbeat/<instant>' at
generation <gen>.
Attempt 1. Retrying: {"code":412, ... "reason":"conditionNotMet"}
```
`HoodieHadoopStorage.delete` reports a failed delete by throwing
`HoodieIOException` when the object
still exists afterwards, which is exactly the outcome here. That exception
is unchecked, so it escapes
the `IOException`-only catch in `WriterHeartbeatUtils.deleteHeartbeatFile`,
propagates out of
`BaseHoodieWriteClient.postCommit` and aborts `commitStats` — after
`commit()` has already completed.
The write is on storage, but the caller is told the round failed.
### Summary and Changelog
Users no longer see a successful commit reported as a failure because of
heartbeat cleanup.
- `HoodieHeartbeatClient.stopHeartbeatScheduler`: `shutdown()` plus
`awaitTermination()` before the
caller deletes the heartbeat file, so a refresh in flight completes first
and the delete then reads
a settled object. `shutdownNow()` interrupts rather than waits, which is
what left the window open.
The await is deliberately **not** placed in `shutdownHeartbeatScheduler`,
because the missed-refresh
path in `updateHeartbeat` calls that from the scheduler thread and must
not await itself. The bound
is `heartbeatWriteTimeoutMs + heartbeatIntervalInMs`, since an in-flight
tick can be parked on the
bounded write.
- `WriterHeartbeatUtils.deleteHeartbeatFile`: also catch
`HoodieIOException`. By the time `postCommit`
runs the cleanup the commit is durable, and no caller consumes the return
value
(`HoodieHeartbeatClient.stop`, `BaseHoodieTableServiceClient` x2 all
discard it), so a failed cleanup
of a transient lease marker must never be fatal. The existing `log.error`
is kept, so a leaked file
is still visible.
The two changes are complementary rather than redundant: a write that
exceeds `heartbeatWriteTimeoutMs`
is deliberately abandoned by `writeHeartbeatFile`, and an abandoned write
can still land after the
delete. The await closes the ordering race for refreshes that complete
normally; the widened catch is
what keeps the abandoned-write tail non-fatal.
Test: `TestHoodieHeartbeatClient#testStopWaitsForInFlightHeartbeatRefresh`
gates a scheduled refresh
mid-write, asserts `stop()` parks rather than deleting, and asserts no
refresh is recorded after the
delete. It fails on current master (the stopper never parks) and passes with
this change.
### Impact
No public API change. `deleteHeartbeatFile` keeps its signature and its
documented "returns whether
the file was deleted" contract; this makes the implementation honour that
contract instead of sometimes
throwing. `stop()` may now block briefly behind an in-flight heartbeat
refresh, bounded as described
above.
### Risk Level
low
Confined to the heartbeat classes. No new locks and nothing held across
storage IO. The widened catch is
on a path whose result every caller discards. Verified with JDK 11: module
compiles, 0 checkstyle
violations, and all 9 tests in `TestHoodieHeartbeatClient` pass, including
the two existing
scheduler-liveness tests (`testSlowHeartbeatWriteDoesNotBlockScheduler`,
`testScheduledHeartbeatRetriesAfterWriteFailure`). The new test was
confirmed to fail against
unmodified master.
### Documentation Update
none — no new config, no default changed, no user-facing behaviour beyond
the removal of a spurious
failure.
### Contributor's checklist
- [x] Read through [contributor's
guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable
--
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]
