strelok89 opened a new pull request, #3833:
URL: https://github.com/apache/celeborn/pull/3833
### What changes were proposed in this pull request?
This revives #3736 by @afterincomparableyum, which was closed by the stale
bot rather than
rejected. Rebased onto current `main`, credit preserved via `Co-Authored-By`.
A worker serializes an empty `RoaringBitmap` as an empty protobuf
bytestring, and
`Utils.byteStringToRoaringBitmap` deserializes empty bytes back to `null`.
That `null` was
stored in `CommitFilesResponse.committedMapIdBitMap` (a `HashMap`, which
allows null values)
and then merged into the `ConcurrentHashMap`
`ShuffleCommittedInfo.committedMapIdBitmap` via
`putAll`, which rejects null values and throws NPE in
`CommitHandler.processResponse`.
Since `ReducePartitionCommitHandler.tryFinalCommit` had no `try/finally`,
the shuffle was
left in `inProcessStageEndShuffleSet` and `setStageEnd` was never called, so
every
`GetReducerFileGroup` request parked until the executor RPC timeout, hanging
the Spark
client process.
Fixes:
- `ControlMessages`: skip null bitmaps when deserializing
`CommitFilesResponse` so `null`
never reaches `ConcurrentHashMap.putAll`.
- `ReducePartitionCommitHandler`: wrap the commit in `try/finally` so the
in-process
stage-end marker is always cleared, allowing a retry.
- `CelebornInputStream.skipLocation`: treat a null mapId bitmap as "do not
skip". This also
fixes a latent NPE independent of the empty-bitmap path: `CommitHandler`
already calls
`setMapIdBitMap` with `committedMapIdBitmap.get(id)`, which is `null`
whenever the id has
no entry.
The null-means-unknown semantics are kept deliberately. The wire format
collapses null and
empty to `ByteString.EMPTY`, so making `byteStringToRoaringBitmap` return an
empty bitmap
would turn "unknown" into "no map ids here" at the `PbSerDeUtils` call sites
and make
`skipLocation` skip locations that must be read.
### Why are the changes needed?
Without this, Spark clients can hang for a long time and emit repeated NPEs.
### Does this PR resolve a correctness bug?
- [x] Yes
### Does this PR introduce _any_ user-facing change?
- [ ] Yes
### How was this patch tested?
Added a `UtilsSuite` regression test asserting a `CommitFilesResponse` with
an empty bitmap
survives the transport round-trip without null map values. Verified it fails
without the
`ControlMessages` fix and passes with it. `celeborn-client/Test/compile` and
`spotless:check -pl common,client` pass locally; the rest is left to CI.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
