DanielLeens commented on PR #12096: URL: https://github.com/apache/seatunnel/pull/12096#issuecomment-6592034565
@SEZ9 thanks, and sorry for the slow turnaround. I checked your list against `aa30f22ef2bd316cb707d89ee233e183a152a0d5`. The only commits since the last substantive change are `dev` merges (the last real change is `f38eb2822`, the savepoint-retention test), so none of the source items is fixed on this head. The green fork Build does not change that. Going through them: - **F1 (restore target deleted by constructor cleanup) - valid in a narrow case.** The `PipelineState` being restored is already deserialized and passed into the constructor, so deleting its file does not break that restore. When restoring the latest checkpoint it can also never sit in the oldest-first prefix. The gap is restoring an older same-job checkpoint/savepoint while newer ones exist: `restoreCompletedCheckpointRetentionState` can then queue and delete that file. I have not written a test for that case, and I have not confirmed whether any later restore path re-reads it from storage; excluding the restore ID from the candidates is cheap, so I agree it should be done, with a test for `max-retained` already exceeded. - **F2 / F4 (savepoints become prunable) - valid.** Steady state already queued every stored ID, savepoints included, before this PR. What this PR adds is that the restart-time scan makes savepoints already in storage eligible. `PipelineState` carries only jobId/pipelineId/checkpointId/states, so telling a savepoint from a checkpoint currently means deserializing the payload, which conflicts directly with F5. The fix needs a lightweight listing or type signal from the storage layer, not just a filter. - **F5 (full payload deserialization in the constructor) - valid, and the most serious one.** It is the same item as P0 Issue 1 in my own review: `getCheckpointsByJobIdAndPipelineId` returns every payload as `byte[]` before anything is pruned. - **F3 (partial batch failure) - valid.** Same as my P0 Issue 2: one permanently undeletable file keeps the whole prefix queued and the queue grows with every checkpoint. It needs per-ID progress and a bounded retry, with a test for a permanent failure followed by deletable files. - **F7 (only `CheckpointStorageException` caught) - valid.** The constructor-time scan and cleanup catch only that type, so a runtime failure from storage or deserialization escapes the constructor and can abort coordinator recreation during failover. Retention here is best effort and must not block recovery. - **F6 / F8 (docs and compatibility note) - valid, nothing written yet.** No `docs/en` or `docs/zh` change exists for the strict `max-retained` window (previously up to `2N-1` between batches), startup pruning, or the extra per-checkpoint `deleteCheckpoint` call. My review also lists two smaller open items: retrying an incomplete startup inventory, and a Javadoc on `cleanExpiredCheckpointsSafely`. So this needs a code change, and I will not claim it is ready. F5 and F2/F4 are coupled through the storage listing contract, so I want to settle that design first rather than patch them separately. On the missing Apache Build check, thanks for taking that on your side. -- 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]
