Copilot commented on code in PR #3370:
URL: https://github.com/apache/fluss/pull/3370#discussion_r3296051518
##########
fluss-lake/fluss-lake-paimon/src/main/java/org/apache/fluss/lake/paimon/tiering/PaimonLakeCommitter.java:
##########
@@ -153,10 +153,6 @@ public LakeCommitResult commit(
}
} catch (Throwable t) {
Review Comment:
With the automatic `tableCommit.abort(...)` removed here, failures during
`commit()` will no longer clean up the `fileCommittables()` unless the caller
explicitly invokes `LakeCommitter.abort`. In the Flink tiering path
(`TieringCommitOperator.commitWriteResults`), `abort` is not called when
`lakeCommitter.commit(...)` throws, so written-but-uncommitted files may be
orphaned. Please either (a) ensure the caller aborts on commit failures when it
is safe, or (b) add a brief comment here documenting that cleanup is
intentionally deferred/omitted to avoid deleting possibly-committed data.
##########
fluss-lake/fluss-lake-paimon/src/main/java/org/apache/fluss/lake/paimon/tiering/PaimonLakeCommitter.java:
##########
@@ -153,10 +153,6 @@ public LakeCommitResult commit(
}
} catch (Throwable t) {
- if (tableCommit != null) {
- // if any error happen while commit, abort the commit to clean
committable
- tableCommit.abort(manifestCommittable.fileCommittables());
- }
throw new IOException(t);
Review Comment:
The catch path no longer aborts (intentionally), but it also doesn't clear
`currentCommitSnapshotId`. If `tableCommit.commit(...)` invokes the callback
and then throws, the ThreadLocal can retain a stale snapshot id for this
thread, which can affect subsequent commits. Consider clearing
`currentCommitSnapshotId` in a `finally` (or in the catch before rethrow) to
avoid leakage/stale state.
--
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]
