corgy-w commented on PR #12440: URL: https://github.com/apache/seatunnel/pull/12440#issuecomment-6112202190
@DanielLeens thanks for the detailed review — all four findings are addressed in `393edcfcb9`: 1. **Issue 1 (visibility of an exactly-once job that commits nothing).** `HudiRecordWriter.close()` now logs the "instant was never committed" case at **ERROR** instead of WARN, with the same guidance (checkpoint enabled / last checkpoint completed). On top of that, `HudiSinkAggregatedCommitter` now tracks the instants it aggregated for a checkpoint, clears them on commit/rollback/skip, and in `close()` logs an ERROR listing the instants that were handed over but never committed — so the condition is surfaced on the committer side as well, which is the one that actually holds the pending state after a restart. 2. **Issue 2 (`init()` leaking a client on retry).** `init()` now closes a previously created `writeClientProvider` before replacing it, with a comment pointing at the `SinkAggregatedCommitter` contract that allows `init()` to be called again on a retry. Covered by a new case in the existing test class, `HudiTwoPhaseCommitTest#shouldReleaseThePreviousClientWhenTheCommitterIsInitializedAgain`: it creates a real local Hudi table, calls `init()` a second time on a live committer and then commits the pending instant. 3. **Issue 3 (timeline reload per commit info).** `commit()` and `abort()` now flatten the aggregated commit infos once, reload the active timeline **once per call** and pass it into `commitInstant`/`rollbackInstant`, so the metadata IO no longer scales with the number of writers. Behavior is unchanged: a stale entry in the preloaded timeline can only make a commit of a *different* instant proceed, and each instant still gets its own existence check. 4. **Issue 4 (`abort()` javadoc).** Rewritten to match the contract: it states that this is currently only invoked by Spark, that Zeta and Flink restart the pipeline instead, and points at the writer heartbeat plus the `LAZY` failed-writes cleaning policy as the actual safety net for the instants of a dead writer. I also removed the unused `org.apache.seatunnel.connectors.seatunnel.hudi.state` package (the dead duplicate `HudiCommitInfo`/`HudiAggregatedCommitInfo`/`HudiSinkState` you flagged in 2.1) — nothing imports it, and dropping it removes the shadowing with the live `hudi.sink.state` types. Local verification on the new head (JDK 11): * `./mvnw -pl seatunnel-connectors-v2/connector-hudi spotless:apply` → clean * `./mvnw -o -pl seatunnel-connectors-v2/connector-hudi test -DforkCount=0` → BUILD SUCCESS, **25 tests, 0 failures** (`HudiTwoPhaseCommitTest` 4/4, including the new retry case) CI is re-running on the new head; I will report the result here once it finishes. -- 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]
