Re: [PR] [core] Do not expire the snapshot the LATEST hint points to [paimon]

["dev-donghwan (via GitHub)" <gi...@apache.org>]

dev-donghwan commented on PR #10323:
URL: https://github.com/apache/paimon/pull/10323#issuecomment-5981468508

   Thanks for the direction. I force-pushed the PR to implement B only, so the read-side
   recovery commits are gone and reads and commit-user deduplication are unchanged from master.
   
   **What this PR changes**
   
   - Snapshot expiration caps the end of the expiration range at the LATEST hint, like the
     consumer boundary, so the hinted snapshot and all later ones are kept.
   - The hint is read strictly right before deleting. If it cannot be read, nothing is deleted
     this time.
   - If the hinted snapshot is already missing, the snapshot after it and all later ones are
     kept. If that one is missing too, nothing is deleted.
   - The protection is skipped when the catalog manages the snapshots.
   - Warnings are logged on expiration while the hint is behind, and on the commit retry path
     that finds the snapshot already committed.
   
   **How each of your points is addressed**
   
   1. **Cap the entire prefix, keep the contiguous suffix.** The cap applies to the whole range,
      so retaining N while deleting N+1 cannot happen.
      (`testHintedSnapshotIsNotExpired`)
   2. **Apply the cap before any deletion, including the time-retention paths.** Both the
      time-retention early return and the retained-count path call `expireUntil`, and the cap
      is applied there before `innerExpireUntil` starts deleting data, changelog and manifest
      files. (`testBothExpirationPathsKeepTheHintedSnapshot`)
   3. **Keep EARLIEST consistent.** EARLIEST points to the first retained snapshot.
      (`testEarliestHintMatchesTheKeptSnapshots`)
   4. **Distinguish absent and unreadable hints, skip when the boundary cannot be
      established.** Following `readOverwrittenFileUtf8`, the hint is returned as an `Optional`
      (present or absent), and an unreadable hint throws an `IOException` after the same
      retries as `readHint`. Then nothing is deleted. A hint that is not a positive number is
      treated as absent, like `findLatest` does. (`testUnreadableHintSkipsExpiration`,
      `testTransientHintReadFailureIsRetried`, `testUnusableHintIsIgnored`)
   5. **Test coverage.**
      - hint write failures and recovery: `testCommitsGoOnWhileHintWritesFail`,
        `testExpireResumesAfterHintWritesRecover`
      - concurrent commit and expiration: `testCommitDuringExpiration`
      - rollback: `testRollback`, `testRollbackDuringExpiration`
      - replay in both conflict-check modes: `testReplayCommittedCommit`, and
        `CommitterOperatorTest#testReplayEndInputCommitWhileLatestHintIsBehind` for the Flink
        batch restart path
      - streaming startup before the hint is repaired: `testStreamingReadFromLatest`
      - timestamp-range reads: `testIncrementalBetweenTimestamps`
   6. **Skip the policy only when the catalog supplies the snapshot state.** It uses the same
      condition as `CatalogEnvironment#snapshotCommit`.
      (`testCatalogManagedSnapshotsIgnoreTheHintFile`)
   7. **Expiration warning.** It includes the hinted ID, the actual latest ID and the retained
      boundary, and is logged once per hint value. The warning for an unreadable hint is logged
      once until the hint can be read again. (`testExpirationWarnsOncePerHint`,
      `testReadFailureWarnsOnceUntilTheHintCanBeRead`)
   8. **Retry warning.** It is logged only on the retry path that finds the snapshot already
      committed, with the snapshot ID and "The LATEST hint may not have been updated." That
      path is also reached when the atomic commit fails without an exception, so the wording
      does not claim one. (`testRetryWarningOnlyWhenTheCommitIsFoundCommitted`)
   
   **In addition**
   
   I handled the case where the hinted snapshot is already missing. Without it, expiring the
   snapshot after it as well makes the table stuck, and on an already stuck table, expiration
   deletes only part of the data files, so the remaining older snapshots can no longer be read.
   (`testHintOnMissingSnapshotKeepsTheNextOne`,
   `testHintOnMissingSnapshotKeepsDataFilesOfTheNextOne`,
   `testHintOnMissingSnapshotsKeepsDataFilesOfOlderSnapshots`)
   
   **Behavior changes**
   
   While the hint is behind, more snapshots than `snapshot.num-retained.max` can be retained,
   like with consumers, and the `expire_snapshots` procedures can expire fewer snapshots. Each
   expiration also reads the LATEST hint once more; it is read right before deleting to keep the
   window against concurrent commits and rollbacks small. These are also listed in the PR
   description.
   


-- 
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: issues-unsubscribe@paimon.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org