SEZ9 commented on PR #12096: URL: https://github.com/apache/seatunnel/pull/12096#issuecomment-6568843594
Thanks for the update on this head (`aa30f22ef2bd316cb707d89ee233e183a152a0d5`). The exact-head fork Build run (41162813818) finishing green is good to see, but as noted in the existing review (5721308646) it does not close out the source-level findings, which are still open: **Blocking (HIGH)** - **F1 – Constructor-time retention cleanup can delete the checkpoint/savepoint being restored from.** Please make sure the restore target is excluded from the retention candidate set (or defer cleanup until after restore completes), and add a test covering restore-from-savepoint/checkpoint with `max-retained` already exceeded. **Should be fixed before merge (MEDIUM)** - **F2 / F4 – Retention restore enumerates all durable pipeline states, including savepoints.** User-triggered savepoints must not become eligible for retention deletion after a restart or failover. Please filter to checkpoints only (or otherwise distinguish savepoints) when rebuilding the retention list. - **F3 – Retry under partial batch failure.** With storage failures now surfaced, a partially-deleted batch can leave cleanup permanently stuck if `deleteCheckpoint` isn't idempotent. Please either make the per-checkpoint delete idempotent (missing file = success) or track per-checkpoint completion so retries only touch what's left. - **F7 – Only `CheckpointStorageException` is caught.** Any `RuntimeException` from storage escapes the `CheckpointCoordinator` constructor and can abort coordinator recreation during active-master failover. Please broaden the handling so retention restore/cleanup failures are logged and don't prevent recovery. - **F5 – Full state payload deserialization in the constructor.** Restoring retention metadata shouldn't require synchronously deserializing every retained checkpoint's state. If a lightweight listing/metadata path is available, please use that, or move the work off the constructor path. - **F6 – Docs.** The retention/cleanup semantics for `checkpoint.storage.max-retained` changed; please update the user-facing documentation accordingly. **Nice to have (LOW)** - **F8 – Compatibility note.** Retention is now exactly `max-retained` (previously up to `2*max-1`) and there is one `deleteCheckpoint` storage call per completed checkpoint. A short note in the PR description / docs on this behavior change would help users and storage plugin implementers. On the missing Apache Build check in the PR rollup: that's a separate workflow permission/check discovery matter and doesn't affect the above; I'll follow up on that side. Once a new revision addressing F1 and the F2/F4/F3/F7 items is pushed, I'll re-review promptly. <!-- streview-comment:2226 --> -- 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]
