RexXiong commented on PR #3672:
URL: https://github.com/apache/celeborn/pull/3672#issuecomment-4645187726
Thanks for the work on this PR. The iterator-fully-consumed check is a good
defensive guard, but it's worth noting that this only verifies the **read
side** — i.e., all records were consumed from the iterator. It does not
guarantee that all consumed records were actually **shuffled out** to the
remote shuffle service successfully.
The write pipeline has multiple stages where records could be silently lost
after being consumed:
```
iterator.next() → serialize to buffer → dataPusher async push →
shuffle service worker
↑
PR checks here records could be
lost anywhere after
```
A stronger correctness guarantee would be to verify that the number of
records successfully written (e.g., `writeMetrics.recordsWritten()`) matches
the number of records consumed from the input iterator. This would catch silent
drops within the client-side pipeline (buffer management bugs, serialization
edge cases, etc.).
Something like:
```java
if (inputRecordCount != writeMetrics.recordsWritten()) {
// fail the task before mapperEnd
}
```
Note that for `mapSideCombine`, the comparison should be done against the
combined iterator's output count rather than the raw input count.
Would you consider adding a record count cross-check in this PR or as a
follow-up JIRA?
*Reviewed with Claude Code*
--
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]
