SEZ9 commented on PR #11827: URL: https://github.com/apache/seatunnel/pull/11827#issuecomment-5358317124
Thanks @surafel58 and @DanielLeens for the follow-through here, and glad the flaky Build check sorted itself out on re-run. Before this gets merged, I'd like to close the loop on the three points from my earlier review — I can't tell from the thread whether they were addressed on the current head (`5d809f69`) or deliberately deferred: 1. **Replay-safety comment in `prepareCommit()`** (`PrometheusWriter.java`): the comment overstates Prometheus remote-write idempotency. If the receiver rejects duplicate/out-of-order samples on replay, a restored job can land in a checkpoint-fail crash loop. Could you either soften the comment to reflect that, or explain how replayed writes are tolerated? 2. **No retry/backoff on the checkpoint flush** (`PrometheusWriter.java`): the flush is a single-shot HTTP call, so a transient Prometheus outage now fails every checkpoint and can kill the job on Flink/Spark. A small bounded retry with backoff (or a pointer to where this is already handled) would resolve this. 3. **Test gap** (`PrometheusWriterTest.java`): the new `prepareCommit` test doesn't verify the buffer is cleared after the checkpoint flush, so double-delivery on a second checkpoint would go undetected. An assertion on buffer state after flush would cover it. If any of these were already handled on `5d809f69`, a quick pointer to the relevant change is all I need. Items 1 and 2 are the ones I'd like resolved (or explicitly justified) before merge; item 3 is a small test addition. Thanks again for the careful iteration on this one! <!-- streview-comment:392 --> -- 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]
