[Feature] use mergeData for bolt NativeCelebornClient (#370)#590
Draft
afterincomparableyum wants to merge 1 commit into
Draft
[Feature] use mergeData for bolt NativeCelebornClient (#370)#590afterincomparableyum wants to merge 1 commit into
afterincomparableyum wants to merge 1 commit into
Conversation
Author
|
Note, I will remove the boolean flag that gates push merged and have it turned on by default, I will also update the Conan files, e2e files, etc with the latest commit on celeborn main once my PR to fix a bug on celeborn cpp client gets merged. Overall, the core logic of this PR is ready though |
Author
|
Sorry, I squashed the commits with my fork celeborn repo (hence why CI is failing now), let me update this draft PR with the squashed commit hash. |
5b9206b to
7d4b1fc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Bolt's
CelebornPartitionWriterrouted every per partition payload throughRssClient::pushPartitionData-->ShuffleClient::pushData, regardless of size. Celeborn's Java client (and Gluten's RSS integration) instead usesmergeData/pushMergedDatafor small payloads, letting the client coalesce many small batches into one network flush per worker. This significantly reduces RPC count and improves throughput for workloads with many small partitions.The C++
NativeCelebornClientdid not exposemergeData, so Bolt's native shuffle missed this optimization. This PR adds the missing path, gated bypushBufferMaxSize, and matches the Java client's behavior.Issue Number: close #370
Type of Change
Description
Production code
RssClient.h: added virtualmergePartitionData(partitionId, bytes, size)to the abstract base. Default implementation delegates to
pushPartitionData, so non-Celeborn backends keep working unchanged.NativeCelebornClient.{h,cpp}: implementedmergePartitionDataby calling the upstream cpp-client'sShuffleClient::mergeData. LikepushPartitionData, it rejects calls afterstop().CelebornPartitionWriter.cpp: per partition payloads are now routed by size and a newcelebornMergeEnabledoption. Payloads<= pushBufferMaxSizego throughmergePartitionData(coalesced), larger payloads bypass the merge buffer and go throughpushPartitionDatadirectly. Matches Gluten's Java client behavior.Options.h: addedPartitionWriterOptions::celebornMergeEnabled(defaulttrue). Lets callers opt out to the legacy push-only path for debugging or A/B comparison.Dependency
celeborn-cpp-clientConan recipe to a version that exposesShuffleClient::mergeDataand the updatedCelebornInputStream/WorkerPartitionReaderinterfaces.Tests
CelebornClientTest.cpp:FakeShuffleClientnow implementsmergeData(counted separately) andpushMergedData, plus theexcludeFailedFetchLocationoverride required by the cpp-client bump.NativeCelebornClientTest.mergePartitionDataRoutesToMergeDataverifies the C++ wrapper callsShuffleClient::mergeData(notpushData) and rejects calls afterstop().RssClientTest.defaultMergeDelegatesToPushwhich verifies non-overriding backends fall through topushPartitionData.main()that callsfolly::initso the cpp-client's new folly-singleton use inWorkerPartitionReader::initAndCheckinitializes correctly. Replaces the linkedGTest::gtest_main.ShuffleMatrixTest.cpp/ShuffleTestBase.{h,cpp}:bool celebornMergeEnabledtoShuffleTestParam.{true, false}forkCelebornwriter cases (kLocal stays at the default), giving end-to-end parity coverage of the merge vs legacy push paths against a real Celeborn cluster.tests/celeborn/scripts/run_e2e.sh: defaults updatedRelease Note
Release Note:
Checklist (For Author)