xumingming opened a new pull request, #3716: URL: https://github.com/apache/celeborn/pull/3716
### What changes were proposed in this pull request? Celeborn's E2E integrity check computes CRC_M inside `ShuffleClientImpl.pushOrMergeData()`, which runs in the async `DataPusher` thread. This leaves the segment from batch assembly in the writer thread through the `DataPusher` queue entirely outside the checked zone — meaning any corruption that occurs in that window is invisible to the integrity check and reaches reducers silently. This change closes that gap and enables detection of a class of correctness bugs where data corruption occurs between batch assembly and async push dispatch, including bugs involving shared buffer pool references. Introduce `ShuffleClient.computeBatchCRC()` and consolidate its invocation into two choke points: - `DataPusher.addTask()`: covers all async push paths. The CRC is recorded on the writer thread immediately before the buffer is enqueued, so `DataPusher.pushData()` intentionally uses the bare `client.pushData()` to avoid double-counting the same batch into CommitMetadata. - `ShuffleClient.pushDataWithCRC()` and `ShuffleClient.mergeDataWithCRC()`: new concrete convenience methods that call `computeBatchCRC()` then delegate to the abstract `pushData()`/`mergeData()`. These cover all synchronous push paths (`pushGiantRecord`, `close()` flush). The abstract `pushData()`/`mergeData()` are now documented as internal-use-only; all writer call sites across `HashBasedShuffleWriter` (spark-2/3), `SortBasedShuffleWriter` (spark-2/3), and `SortBasedPusher` use the `WithCRC` variants instead. The now-redundant CRC computation inside `pushOrMergeData()` is removed. This consolidation eliminates 7 scattered `computeBatchCRC` call sites that previously had to be manually paired with each push/merge call, reducing the risk of a future call site omitting the CRC step. ### Why are the changes needed? Enhance E2E Integrity Check, so it can cover more code path. ### Does this PR resolve a correctness bug? - [ ] Yes ### Does this PR introduce _any_ user-facing change? - [ ] Yes ### How was this patch tested? Unit Test. -- 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]
