JingsongLi commented on code in PR #9437:
URL: https://github.com/apache/paimon/pull/9437#discussion_r4051988881
##########
paimon-core/src/main/java/org/apache/paimon/utils/SnapshotManager.java:
##########
@@ -147,13 +152,44 @@ public Snapshot tryGetSnapshot(long snapshotId) throws
FileNotFoundException {
public boolean snapshotExists(long snapshotId) {
Path path = snapshotPath(snapshotId);
- try {
- return fileIO.exists(path);
- } catch (IOException e) {
- throw new RuntimeException(
- "Failed to determine if snapshot #" + snapshotId + "
exists in path " + path,
- e);
+ IOException failure = null;
+ for (int attempt = 1; attempt <= SNAPSHOT_EXISTS_MAX_ATTEMPTS;
attempt++) {
Review Comment:
[P2] Split the unrelated `snapshotExists` retry into its own PR
This change also rewrites `snapshotExists` to retry `fileIO.exists` three
times with a 1s blocking sleep (`SNAPSHOT_EXISTS_MAX_ATTEMPTS`,
`SNAPSHOT_EXISTS_RETRY_INTERVAL_MILLIS`). It is unrelated to the
duplicate-timestamp lookup, is not mentioned in the PR description, and has no
test. It applies to a method with a dozen-plus production call sites, including
commit paths (`StrictModeChecker`, `IcebergCommitCallback`), streaming poll
loops (`NextSnapshotFetcher`, `AppendCompactCoordinator`, `TagAutoCreation`)
and branch/tag maintenance (`FileSystemBranchManager`, `TagManager`).
**Trigger**: any caller where `fileIO.exists` throws persistently
(permission or short-read errors, an object store under throttling).
**Impact**: every `snapshotExists` call then blocks up to 2s, logs two WARNs
and issues 3x the number of `exists` requests before failing with the same
`RuntimeException` as before. So a focused race-fix PR silently changes failure
latency and request volume for commit/streaming paths. Note that the lookup fix
retries because it must re-resolve concurrent rollback/expiration; this retry
only re-issues an identical `exists`, so its benefit is limited to transient FS
errors, which one would not expect on the same path the PR is about.
**Fix**: keep this PR to the duplicate-timestamp lookup and move the retry
to a separate PR with its own rationale and test (e.g. a `FileIO` that throws
once and then succeeds). If it is kept here, please add a test and consider a
shorter interval (the existing `tryFromPath` retry uses 200ms).
--
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]
