Stephen0421 commented on PR #10062: URL: https://github.com/apache/paimon/pull/10062#issuecomment-5892414424
> Requirement fit: SUPPORTED. Spark users have a real end-to-end need to expire separated changelogs. > > Implementation: FINDINGS. The previous binary-compatibility P1 is fixed: `javap -s` on this head shows `ExpireChangelogImpl.expireAll(): ()V` unchanged, with the new `expireAllDeletedCount(): ()I` alongside it; the Flink callers still compile and CI is green. The previous missing/skip case for `delete_all` is also addressed. > > [P2] Make `deleted_changelogs_count` count actual removals in the regular procedure path (`paimon-spark/paimon-spark-common/src/main/java/org/apache/paimon/spark/procedure/ExpireChangelogsProcedure.java:118-119`). That path returns `ExpireChangelogImpl.expire()`, whose `expireUntil()` skips a missing changelog or file-skipper failure but still returns the full ID-range length (`ExpireChangelogImpl.java:177-201`). I strengthened the existing `ChangelogExpireTest#testExpireWithMiddleChangelogNotFound` only in an isolated checkout to compare the returned count with changelog files actually removed: this head reported **10** while **9** were removed. The new Spark result column and documentation claim a successful deletion count, so an operator can falsely conclude cleanup completed. Count actual successful removals in both paths, or explicitly change the output contract to a processed ID-range count. Also note that `delete_all` increments its new count after `FileIO.deleteQuietly() `, which swallows deletion failure; that path needs the same success check under an I/O failure. > > Verification on `34adaec`: `ChangelogExpireTest` 9/9 passed; Spark 3 `ExpireChangelogsProcedureTest` 7/7 passed; all current CI checks passed. The focused count audit failed exactly as above and was reverted from the isolated checkout. The count contract still needs repair before production use. Thanks for the review. The count contract is now actual successful removals on both paths. **Regular expire path:** `expireUntil()` no longer returns the ID-range length. A missing changelog or a file-skipper failure is not counted. `ChangelogExpireTest#testExpireWithMiddleChangelogNotFound` now asserts the returned count equals the changelog files actually removed. The Spark `retain_max` path has the same case: with one missing changelog it returns `1` instead of `2`. **`delete_all`:** `expireAllDeletedCount()` uses the same check. `FileIO.deleteQuietly()` still swallows I/O errors, and a changelog file that is still present afterwards is not counted. `testDeleteChangelogFileRequiresActualRemoval` covers a deletion that fails and leaves the file in place. **Argument precedence:** explicit `retain_max` / `retain_min` / `max_deletes` are merged into the dynamic options before `table.copy()`, so schema validation sees the final config. For a table with `changelog.num-retained.min=4`, `options => 'changelog.num-retained.max=2'` together with `retain_max => 8` is valid and runs as min=4, max=8. -- 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]
