DanielLeens commented on PR #10755: URL: https://github.com/apache/seatunnel/pull/10755#issuecomment-6210063409
Thanks @zhengxiang378928908-code for offering to help here, and thanks @chl-wxp for starting this fix. The large-batch OOM in #10623 is a real problem, and the direction of rotating the TaskWriter is worth continuing. A quick status on the head `1540446d` (unchanged since my 2026-04-12 review; GitHub also reports it as blocked, and @davidzollo asked for a conflict resolution on 08-02): the two blockers I raised are still open. `flush_row_size` has no `> 0` validation (a value of 0 or less makes the threshold true before the first row), and the new option has no docs in `docs/en` / `docs/zh` and no focused test. I also owe you a correction. Sorry for missing this in my first pass: the rotation interacts with upsert mode. `IcebergRecordWriter.write()` now does `flush()` + `resetWriter()` once `currentCount >= flushRowSize`. In upsert mode the writer is a `BaseDeltaTaskWriter` built on Iceberg's `BaseEqualityDeltaWriter`, and as far as I can tell from reading the code (this is my inference, I have not run it) the in-writer bookkeeping of rows inserted earlier in the same writer is lost on rotation. A DELETE or UPDATE for a key inserted before the rotation, but within the same checkpoint, would then be written as an equality delete that sits in the same commit as the data file it is meant to remove, and Iceberg does not apply an equality delete to data files of the same sequence number. That could leave a stale row. Whoever continues this should either keep the rotation append-only, or rotate only at a point where in-flight keys cannot cross it, and add a test for the INSERT -> rotate -> DEL ETE/UPDATE sequence. This matches what @zhengxiang378928908-code described as "a bounded writer rotation that stays correct in upsert mode". On ownership: it is entirely up to @chl-wxp. If you plan to continue, please rebase onto the latest `dev` and address the items above. If you do not have time, taking the PR over as proposed (keeping the original author as co-author, and referencing this PR and #10623) sounds good to me; I would suggest waiting for the week mentioned in the proposal so the two of you do not end up with duplicate PRs. In both cases I am happy to re-review the full diff once a new head is pushed. -- 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]
