LuciferYang opened a new pull request, #3826: URL: https://github.com/apache/celeborn/pull/3826
### 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? <!-- Check if yes. The `correctness` label will be added/removed automatically. --> - [ ] Yes ### Does this PR introduce _any_ user-facing change? <!-- Check if yes. --> - [ ] 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. -- 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]
