xumingming commented on PR #3716: URL: https://github.com/apache/celeborn/pull/3716#issuecomment-4612646210
@SteNicholas Thanks for the review, the following is my response to each of the question you raised: > 1. The mapperEnded check is no longer atomic with the push decision. The race between computeBatchCRC (writer thread) and pushOrMergeData (DataPusher thread) **cannot cause a false integrity failure because:** 1. All three operations — computeBatchCRC, pushOrMergeData, and mapperEnd — are scoped to the same (shuffleId, mapId, attemptId) via PushState. 2. If mapperEnded flips to true between CRC accumulation and the actual push, the current attempt is the losing attempt (another attempt won). 3. **The losing attempt never sends MapperEnd, so whatever CRC it accumulated (complete or partial) is never transmitted to the server and never validated.** 4. Only the winning attempt's MapperEnd — carrying its own full, correct CRC — is validated. > 2. Accumulation now races PushState.cleanup() across threads. addTask modifies PushState.commitMetadataMap and PushState.cleanup() modifies PushState.inFlightRequestTracker, there is actually no races. Even if we clear PushState.commitMetadataMap in PushState.cleanup() in the future, it does not matter because when cleanup is called it means what ever accumulated in PushState is not needed anymore. > DummyShuffleClient.getPushState changes from returning a fresh PushState each call to a cached one. DummyShuffleClient is used for tests, right? So as long as all tests pass, there will be no issue, right? > Javadoc: pushDataWithCRC says "Prefer this over pushData at all writer call sites," but DataPusher deliberately uses bare pushData. A one-line cross-reference would remove the apparent contradiction. I have commented why we need to call pushData inside DataPusher, do we need more comment description? > mergeData's "internal use only / async push pipeline" note is copied from pushData, but mergeData isn't driven by the async DataPusher — all its callers should use mergeDataWithCRC. Yes, you are right about this. The basic idea is that mergeData/pushData should only be used internally, do you want me to further optimize the comment? > The one gap maps to point https://github.com/apache/celeborn/issues/1: no test exercises mapperEnded flipping between addTask and the actual push — a targeted unit test there would de-risk the merge. From the reasoning of the first answer, we can see that the MapperEnd event will never be sent for the losing mapper attempt, so it is guaranteed by current logic flow. -- 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]
