jerrypeng commented on PR #56878: URL: https://github.com/apache/spark/pull/56878#issuecomment-4900207714
> Test coverage for this PR is thin relative to what it introduces. The suite only checks the getWriter return type and the memory-overflow guard; the substantive logic (termination-ack seq validation, background error propagation via ErrorNotifier, semaphore back-pressure, cleanup releasing queued/pooled buffers, the checksum path, time-based flush) is deferred to the end-to-end suite in part 7. I understand the stacked-PR approach, but as a standalone merge these paths land in master with no CI coverage. The good news is the writer is already built for isolated testing — serverHandler and errorNotifier are injectable constructor params, onTerminationAckReceived is private[streaming], and shards / allAcksReceived / bufferPool are all exposed for tests — so writer-side unit tests without a real reader look very achievable. Worth adding at least: ack seq-number mismatch → task failure, a send-failure surfacing through ErrorNotifier, cleanup releasing queued buffers, and one c hecksum-enabled round-trip. Added writer-side unit tests (no reader needed) — a send failure surfacing through ErrorNotifier on both the synchronous and asynchronous write-completion paths, cleanupResources releasing queued pooled buffers, and the CRC32C checksum being computed and embedded in the DataMessage on the wire (verified against an independent CRC32C over the payload). The termination-ack seq-validation path is already covered by the end-to-end suite in the tests PR, so I didn't duplicate it here. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
