dev-donghwan commented on PR #10323: URL: https://github.com/apache/paimon/pull/10323#issuecomment-5947044510
Thanks for the detailed review, @JingsongLi. You're right about both points. I reproduced the duplicate commit on `d8ef8b9`, both in core (replay with and without the append-file check) and through the Flink `END_INPUT` path on a `bucket=-1` table. I think I took the wrong approach with this PR, so before pushing anything I'd like to ask for your view as the original author of the hint files. The current direction is to fix the read side. To continue with it, every caller that relies on the latest snapshot id would have to handle a hint that points to an expired snapshot. `latestSnapshotId()` alone has about 50 callers in the main code, and `latestSnapshot()` and `latestSnapshotOfUser()` have more, in core, Flink and Spark. I started with the ones you pointed out and fixed the dedup path, including the `commit.last-safe-snapshot` branch, which removed the duplicate commit. But the tests then showed other callers behaving differently from what they intend, because only `latestSnapshot()` gets the real latest while `latestSnapshotId()` still returns the stale id. For example: - `incremental-between-timestamp` silently reads all retained snapshots instead of the requested range (5 rows instead of 2), where master fails. - `scan.mode=latest` still starts from the stale id, so your second point stays unfixed. - The Flink/Spark rollback procedures call `latestSnapshot()` before `rollbackTo`, which is what currently stops them on a stale hint. With read-side recovery that call succeeds, and they go on to a `rollbackTo` that keeps the snapshots after the target and deletes newer tags. Fixing each of these one by one would make the change much larger, and each fix could bring another side effect like these. So instead of the read side, I looked at the side that deletes snapshots. The stale state only appears when expiration deletes the snapshot the `LATEST` hint points to, so I'd like to propose preventing that instead. **Proposal** Snapshot expiration does not expire the snapshot the `LATEST` hint points to, similar to how it already keeps the snapshots consumers still need. Then `findLatest` works as it is. When the hint is behind, `snapshot-(N + 1)` exists, so it lists the directory and returns the real latest. All read-side changes are reverted, so every caller behaves exactly as on master. - No extra IO on the normal path. Expiration reads the `LATEST` hint once, and only when it is about to delete snapshots. - Expiration logs a warning whenever it keeps snapshots because the hint is behind. - I would skip the check when the catalog provides the latest snapshot itself (e.g. REST), where the `LATEST` file is not the source of truth. - A table that is already in the stale state behaves as on master. Some operations there are already wrong on master today (see below), so I think such tables are better handled by an explicit `repair_latest_snapshot` procedure, similar to `repair_earliest_snapshot` (#8883), as a follow-up. **If hint writes keep failing** In our incident, hint writes failed because of a broken TaskManager, which Paimon cannot prevent. What Paimon can avoid is turning that into a permanent outage: the TaskManager problem lasted about 6 minutes, while the table stayed stuck for two days until `LATEST` was fixed by hand. I ran 30 commits with every `LATEST` write failing. With the proposal, all of them succeeded (each one slower because of the existing hint-write retries), reads and `scan.mode=latest` stayed correct, and snapshots piled up to 31. The first commit after hint writes recovered moved the hint, and expiration cleaned up back to the retention. On master the same run gets stuck once the hinted snapshot is expired. Today the only trace of this is the generic `Retry commit for exception` warning, and the retry path that finds the commit already done logs nothing. If you agree, I'd also add a warning there, e.g. "Snapshot #N was committed by a previous attempt that failed, the LATEST hint may not have been updated", so the problem shows up at the first commit rather than only once snapshots pile up. I also tried letting expiration rewrite the `LATEST` hint before deleting. It repairs the hint when another process runs expiration, but without a compare-and-swap on hint files it can race with rollback and write a hint that points to a snapshot the rollback has just deleted, which is the same stuck state. It also adds the hint-write retry delay to every expiration while writes fail. So I kept the proposal read-only. **Measured comparison** - master: current behaviour - A: read-side recovery (`d8ef8b9` plus the dedup fix) - B: the proposal above All cells come from running the same probe on the four variants, except the rollback procedures, which are from reading the code. *1. A table whose `LATEST` hint stops moving while commits and expiration go on* | | master | A | B | A + B | |---|---|---|---|---| | Hinted snapshot | Expired | Expired | Kept | Kept | | New commit | Fails on every attempt until `LATEST` is fixed by hand | Succeeds | Succeeds | Succeeds | | Committer replay (with/without append-file check) | Fails until `LATEST` is fixed by hand | No duplicate | No duplicate | No duplicate | | `scan.mode=latest` start | Stale id | Stale id | Real next snapshot | Real next snapshot | | `incremental-between-timestamp` | Fails until `LATEST` is fixed by hand | Reads all retained snapshots (5 rows instead of 2) | Correct | Correct | | `rollbackTo` | Returns normally, but keeps the snapshots after the target and deletes newer tags | Same as master | Correct | Correct | | Rollback procedures | Fail at `latestSnapshot()` | Reach `rollbackTo` above | Correct | Correct | *2. A table that is already in the stale state* | | master | A | B | A + B | |---|---|---|---|---| | New commit, committer replay | Fail until `LATEST` is fixed by hand | Succeed, no duplicate | Same as master | Succeed, no duplicate | | `incremental-between-timestamp` | Fails until `LATEST` is fixed by hand | Reads all retained snapshots | Same as master | Reads all retained snapshots | | `scan.mode=latest` start, `rollbackTo` | Wrong already on master | Same as master | Same as master | Same as master | With B in place, A only runs on tables that are already stale. There it brings back commits but turns other failures into silently wrong results, and it does not fix the rest. So I'd go with B alone. **Tests** The tests produce the stale hint through real commits with `LATEST` writes skipped, instead of resetting `LATEST` by hand. They cover the replay in both conflict-check modes, the Flink `END_INPUT` replay, `scan.mode=latest`, `rollbackTo` and `incremental-between-timestamp`. All of them fail on master and pass with B. The paimon-core and flink committer test suites pass as well. Does this direction match how you see the hint files, and would you like the extra warning in the commit retry path as part of this PR? If so, I'll update the PR accordingly. -- 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]
