SteNicholas commented on PR #3721: URL: https://github.com/apache/celeborn/pull/3721#issuecomment-4670175773
Re-checked the latest two pushes. Both points from my earlier review are addressed in `922eba8`: 1. The best-effort-cancel comment in `buildCommitFilesResponseOnCancel` documents the race precisely — `failed` is computed before the committed snapshot and the sets are append-only, so a racing commit can only land in both `failed` and `committed` (safe over-report), never in neither. 2. Empty partitions are no longer reported as failed: `COMMIT_FILE_EXCEPTION` is gated on nothing-committed-and-nothing-empty, with the empty-only unit test to match. Also verified `59c989f`'s fix for the status/snapshot inconsistency Copilot flagged: `status` is now derived from the same committed snapshots returned in the response, so `COMMIT_FILE_EXCEPTION` with non-empty committed lists is no longer possible. The residual window (the empty sets are still read live in the status check) only degrades to a safe over-report of `failed`, consistent with the documented design. One leftover nit: the comment "COMMIT_FILE_EXCEPTION only when nothing committed and nothing empty; empty files are a successful terminal state and must not be reported as failed." appears twice in the helper — above the committed-list snapshot and again above the status computation. Worth dropping one copy before merge. LGTM. -- 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]
