Repository navigation
Conversation
b6b7269 to
737d4fb
Compare
zaynt4606
left a comment
There was a problem hiding this comment.
Thanks for adding the compression-ratio metric. I found two accounting gaps that can make the displayed ratio incorrect. Please address the inline comments and add regression coverage for these paths.
| encodedAttemptId, | ||
| numMappers, | ||
| numPartitions, | ||
| new MapperMetrics(pusher.getUncompressedBytesWritten())); |
There was a problem hiding this comment.
Could we include bytes sent by pushGiantRecord() in this metric? That path pushes directly through shuffleClient.pushDataWithCRC() and increments writeMetrics.bytesWritten, but never updates SortBasedPusher's uncompressed-byte counter, which is the sole source used here. If all records exceed pushBufferMaxSize, this reports zero and the UI shows N/A despite a successful write. Please account for the direct-push path and cover it with a giant-record test.
There was a problem hiding this comment.
Good catch. pushGiantRecord now increments a writer-level giant-bytes counter that is summed with the pusher's counter in the MapperMetrics reported at mapperEnd, mirroring HashBasedShuffleWriter. The writer suites now assert the reported uncompressed bytes equal the bytes that reached the shuffle client — the all-giant-record tests (testGiantRecord, testGiantRecordWithFastWrite, which force every record above pushBufferMaxSize) fail without the fix, and the mixed-path checkWithIntegrity covers giant + buffered records together.
| event match { | ||
| case e: CelebornMapperMetricsEvent => | ||
| if (pluginEnabled) { | ||
| totalUncompressedWriteBytes.addAndGet(e.getUncompressedBytesWritten) |
There was a problem hiding this comment.
Could the numerator and denominator use the same set of mapper attempts? onTaskEnd adds shuffleWriteMetrics.bytesWritten for every task end with metrics, while LifecycleManager.handleMapperEnd emits this event only when finishMapperAttempt accepts that attempt. If a failed or speculative attempt writes data and a later attempt succeeds, its compressed bytes remain in the denominator but its uncompressed bytes are absent from the numerator, understating the ratio. Please align the accounting and test a retry/duplicate-attempt case.
There was a problem hiding this comment.
Aligned. Both sides now use the same set of mapper attempts:
- The listener counts
shuffleWriteMetricsonly for successful task ends (failed/killed attempts push bytes whose MapperEnd is never reported). LifecycleManagerreports metrics for every successfully closed writer attempt — including speculative/rerun attempts whose MapperEnd the commit handler rejects — deduplicated by attemptId so a retried MapperEnd RPC of the same attempt is not counted twice.
Covered by CelebornListenerSuite.shuffleWriteMetricsCountOnlySuccessfulAttempts (success + killed attempts + mapper events) and a LifecycleManagerSuite test driving the accepted / retried / duplicate-attempt cases over real RPC.
fcd341e to
22b8ad3
Compare
Track the bytes each mapper hands to ShuffleClient before compression and show the aggregated compression ratio (uncompressed/compressed) on the Celeborn tab of the Spark UI. - Writers report the bytes at mapperEnd via a new extensible MapperMetrics message carried by PbMapperEnd (field 10), keeping MapperEnd source compatible through a default value. - LifecycleManager forwards the metrics to a registered MapperEndCallback; SparkShuffleManager posts CelebornMapperMetricsEvent on the driver listener bus so the value is available in the live UI and replayable by the History Server from the event log. - CelebornListener aggregates the events and CelebornShufflePage renders the ratio; unreported values degrade to N/A. - Register CelebornPlugin in the docker spark-defaults.conf so the local docker-compose cluster exercises the UI end-to-end.
22b8ad3 to
ea7a4fa
Compare
What changes were proposed in this pull request?
Show the shuffle compression ratio (uncompressed/compressed) on the Celeborn tab of the Spark UI.
PbMapperMetricsmessage carried byPbMapperEndas field 10;MapperEndgains a defaultedMapperMetricsparameter, so future mapper-side metrics go there instead of new top-level fields.ShuffleClientadds amapperEndoverload carryingMapperMetrics(default impl ignores it);LifecycleManagerforwards the metrics to a registeredMapperEndCallback.HashBasedShuffleWriter/SortBasedShuffleWritercount the bytes handed toShuffleClientbefore compression (including the giant-record direct-push path) and report them atmapperEnd;SparkShuffleManagerposts aCelebornMapperMetricsEventon the driver listener bus, so the value reaches both the live UI and the History Server event log;CelebornListeneraggregates the events andCelebornShufflePagerenders the ratio, degrading toN/Awhen not reported (old clients).CelebornPluginin the dockerspark-defaults.confso the docker-compose cluster exercises the UI end-to-end.Numerator and denominator use the same set of mapper attempts: the listener counts
shuffleWriteMetricsonly for successful task ends, and LifecycleManager reports metrics for every successfully closed writer attempt — including speculative/rerun attempts whose MapperEnd the commit handler rejects — deduplicated by attemptId so retried MapperEnd RPCs are not counted twice.Why are the changes needed?
The Spark UI shows the shuffle bytes written to Celeborn (post-compression), but there is no way to see how much the compression actually saves. Exposing the ratio helps users evaluate codec choices and estimate real network/disk traffic during shuffle.
CELEBORN-2479
Does this PR resolve a correctness bug?
Does this PR introduce any user-facing change?
The Celeborn tab of the Spark UI shows a new summary line: "Compression Ratio (uncompressed/compressed)". Shuffle write bytes/time now exclude failed or killed attempts, matching Spark's own stage accounting.
How was this patch tested?
UtilsSuiteMapperEnd ser/de roundtrip withMapperMetrics;CelebornListenerSuiteaggregation, event JSON roundtrip, and failed-attempt exclusion tests; writer suites assert the uncompressed bytes reported at mapperEnd equal the bytes that reached the shuffle client (all-giant-record and mixed cases);LifecycleManagerSuiteexercises the accepted / retried / duplicate-attempt callback behavior over real RPC.