SEZ9 commented on PR #12678: URL: https://github.com/apache/seatunnel/pull/12678#issuecomment-6432439952
Thanks for the rebase — I re-checked the diff at `551187b8a090` against my earlier notes (the content matches `ac4bd22f`, so they carry over). **F1 – popping ids before `deleteCheckpoint` / failure handling** Moving the prune into `pruneExcessCompletedCheckpoints` after `notifyCompleted`, catching `CheckpointStorageException`, re-adding the ids in their original order and only logging a WARN addresses the main concern: a storage hiccup no longer fails the checkpoint and the ids stay tracked so the next completion retries the delete. Three things I'd still like to settle here: - The catch is scoped to `CheckpointStorageException`. Could you confirm what happens if `deleteCheckpoint` surfaces anything else (e.g. a runtime exception from the FS client) — does that still propagate and fail the checkpoint, and if so is that intended? A one-line comment in the helper explaining the choice would be enough. - With `while (size > max)` the prune now runs on every completion in steady state, and `deleteCheckpoint` lists the namespace each time. That's acceptable to me for correctness, but please add a short note in the PR description acknowledging the per-completion listing so the trade-off is on record. - Please add a unit test for the retry path: delete fails once, ids are restored in order, and the following completion deletes the backlog. **F2 – `completedCheckpointIds` not seeded on restore** I don't see this addressed at the current head. After a pipeline restart the deque starts empty, so storage can again hold up to roughly 2x `max-retained` and the pre-restart files are never pruned. Could you either (a) seed `completedCheckpointIds` from the storage listing when the coordinator is constructed in restore mode, with a test asserting the bound holds across a restart, or (b) if you'd prefer to keep this PR focused, say so explicitly here and open a follow-up issue so the limitation is tracked? Either is fine, but the PR description should state which one applies. The `max-retained <= 0` item from my last review still stands as well. Happy to take another look once these are in. <!-- streview-comment:2072 --> -- 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]
