Skip to content

[CELEBORN-2443] Serialize the peer's own mapIdBitMap - #3826

Open
LuciferYang wants to merge 3 commits into
apache:mainfrom
LuciferYang:fix-pbserde-peer-mapid-bitmap
Open

[CELEBORN-2443] Serialize the peer's own mapIdBitMap#3826
LuciferYang wants to merge 3 commits into
apache:mainfrom
LuciferYang:fix-pbserde-peer-mapid-bitmap

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

toPbPartitionLocation fills the peer sub-message from location.getPeer field by field. The mapIdBitmap line was the exception: it read off the root location. This PR points that line at the peer and rewrites the round-trip tests around it.

The tests now cover four cases: a peer with its own bitmap, a root with no bitmap and a peer that has one, a peer with no bitmap that must not inherit the root's, and the same invariant on the packed path (toPbPackedPartitionLocationsPair), which had no bitmap assertion at all. The unpacked tests also check the restored peer's mode and the peer/root back link, neither of which PartitionLocation.equals compares.

Why are the changes needed?

After a round trip, a peer's mapIdBitMap was whatever its root carried. When the root has no bitmap and the peer does, the peer's bitmap disappears: Utils.roaringBitmapToByteString turns both null and empty into ByteString.EMPTY, and byteStringToRoaringBitmap turns EMPTY back into null.

No live path reads the wrong value today. Only CommitHandler.collectResult attaches a mapIdBitMap to a PartitionLocation, and GetReducerFileGroupResponse ships those locations through toPbPackedPartitionLocationsPair, which already writes each location's own bitmap. The remaining callers of the unpacked serializer (PbPartitionSplit.oldPartition, Revive, ChangeLocation, and the non-packed WorkerResource) run at allocation or push time, when the bitmap is still empty. So I left the correctness box unchecked, and I would not backport this until someone shows a path that reaches it.

Does this PR resolve a correctness bug?

  • Yes

Does this PR introduce any user-facing change?

  • Yes

How was this patch tested?

build/mvn test -pl common -Dsuites=org.apache.celeborn.common.util.PbSerDeUtilsTest: 36 tests pass.

I checked both directions by mutating the code. Reverting the production line to location.getMapIdBitMap turns the three unpacked tests red. Changing toPackedPartitionLocation to prefer the peer's bitmap over the location's own turns only the new packed test red.

…ocation

The peer block of toPbPartitionLocation read location.getMapIdBitMap
instead of location.getPeer.getMapIdBitMap, so a round-trip through
fromPbPartitionLocation replaced the peer's own bitmap with a copy of
the primary's, silently discarding the replica's committed mapIds.

All live call sites today serialize allocator-fresh empty bitmaps on
both sides (content-identical), so no behavioral change is expected on
current flows; this fixes the shared serde primitive and adds a guard
test so a future producer carrying divergent committed bitmaps cannot
silently lose the peer's.

Signed-off-by: yangjie01 <yangjie01@baidu.com>
@yew1eb

yew1eb commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

LGTM.

@LuciferYang

Copy link
Copy Markdown
Contributor Author

cc @pan3793 @wForget

@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.78%. Comparing base (90954f7) to head (5c14b92).
⚠️ Report is 27 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3826      +/-   ##
============================================
+ Coverage     58.74%   58.78%   +0.05%     
  Complexity      231      231              
============================================
  Files           399      399              
  Lines         28058    28065       +7     
  Branches       2740     2745       +5     
============================================
+ Hits          16479    16495      +16     
+ Misses        10380    10371       -9     
  Partials       1199     1199              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@wForget wForget left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LuciferYang , LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants