SEZ9 commented on PR #12096: URL: https://github.com/apache/seatunnel/pull/12096#issuecomment-6594401618
Thanks for going through the list against `aa30f22ef2bd316cb707d89ee233e183a152a0d5`. Agreed that with only `dev` merges since `f38eb2822` all eight items are still open on this head. Where I'd like things to land: **F1 (HIGH) – restore target deleted by constructor cleanup.** Agreed on the analysis: the restored `PipelineState` is already deserialized, and the latest checkpoint can never sit in the oldest-first prefix, but restoring an older same-job checkpoint/savepoint while newer ones exist lets `restoreCompletedCheckpointRetentionState` queue and delete that file. This stays the top-priority item since it can remove a user's restore source. Please exclude the restore checkpoint ID from the cleanup candidates and add a test for the case where `max-retained` is already exceeded at restore time. **F2 / F4 – savepoints become prunable after restart.** Agreed. Since `PipelineState` only carries jobId/pipelineId/checkpointId/states, distinguishing a savepoint today means deserializing the payload, which collides with F5. A plain filter won't do; this needs a lightweight listing or type signal from the storage layer. If you have a preferred shape for that, please describe it here before implementing. **F5 – full payload deserialization in the constructor.** Agreed: `getCheckpointsByJobIdAndPipelineId` returns every payload as `byte[]` before anything is pruned. Please move to an ID/metadata-only enumeration, ideally via the same storage-layer change that unblocks F2/F4, so active-master recovery doesn't pay for retained-checkpoint payloads. **F3 – partial batch failure.** Agreed. One permanently undeletable file keeps the whole prefix queued and the queue grows with every checkpoint. Please track per-ID progress with a bounded retry, plus a test covering a permanent failure followed by deletable files. **F7 – only `CheckpointStorageException` caught.** Agreed. Retention is best effort and must not block recovery; please catch and log any runtime failure from storage or deserialization in the constructor-time scan and cleanup so coordinator recreation during failover cannot be aborted by it. **F6 / F8 – docs and compatibility.** Still open. Please update the `checkpoint.storage.max-retained` documentation to reflect the new semantics (exactly `max-retained` kept, one `deleteCheckpoint` call per completed checkpoint, restart-time pruning) and add a short compatibility note for users relying on the previous behavior. Once the storage-layer change for F2/F4/F5 is in place, the rest should follow naturally. Happy to re-review as soon as a new head is pushed. <!-- streview-comment:2251 --> -- 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]
