attiasr opened a new pull request, #3874:
URL: https://github.com/apache/celeborn/pull/3874
### What changes were proposed in this pull request?
`PbSerDeUtils.toPbFileInfo` now adds the MAP partition's writing-segment map
and segment indexes only when they are non-null.
`MapFileMeta` creates both lazily, on the first `addPartitionSegmentId` /
`addSegmentIdAndFirstBufferIndex` call. The worker builds MAP file metas with
`new MapFileMeta()`, so for a MAP partition written without segment granularity
both fields stay `null`, and `putAllPartitionWritingSegment(null)` threw:
```
java.lang.NullPointerException: Cannot invoke "java.util.Map.keySet()"
because "m" is null
at
com.google.protobuf.MapField$MutabilityAwareMap.putAll(MapField.java:312)
at
org.apache.celeborn.common.protocol.PbFileInfo$Builder.putAllPartitionWritingSegment(PbFileInfo.java:1674)
at
org.apache.celeborn.common.util.PbSerDeUtils$.toPbFileInfo(PbSerDeUtils.scala:164)
```
On restore, `fromPbFileInfo` reads both fields back as empty (not null).
That is safe. Non-segment MAP partitions are read through `MapPartitionData`,
which never touches either field. `getSegmentIdByFirstBufferIndex` returns
`null` for an empty index list, and restored metas are already marked
writer-closed, so nothing writes to them. Partitions with segment granularity
serialize exactly as before.
### Why are the changes needed?
When graceful shutdown is enabled, the worker persists committed file infos
through `toPbFileInfoMap` → `toPbFileInfo` in two places, and the NPE breaks
both:
- **The periodic saver**
(`worker-storage-manager-committed-fileinfo-saver`). It runs with
`scheduleWithFixedDelay` and does not catch the NPE, so the first failure
cancels it and nothing is saved afterwards.
- **`saveAllCommittedFileInfosToDB` at shutdown.** It logs the error and
skips any shuffle that contains such a MAP partition.
As a result, after a graceful restart the worker cannot recover MAP
partitions written without segment granularity, such as Flink shuffle
partitions. Readers fail to find them, and the producing tasks have to be
re-run.
### Does this PR resolve a correctness bug?
- [ ] Yes
Shuffle data is never incorrect. Partitions are lost on graceful restart
instead of being recovered.
### Does this PR introduce _any_ user-facing change?
- [x] Yes
With graceful shutdown enabled, a worker restart now recovers committed MAP
partitions without segment granularity. No configuration or protocol change.
### How was this patch tested?
Added two tests to `PbSerDeUtilsTest`:
- `fromAndToPbFileInfo with MapFileMeta without segment granularity` builds
the meta the way the worker does (no-arg constructor plus buffer size and
subpartition count). It round-trips it through `toPbFileInfo`/`fromPbFileInfo`
and through `toPbFileInfoMap`/`fromPbFileInfoMap`, checking the buffer size,
subpartition count, segment-granularity flag, writer-closed flag, a segment
lookup, path and length. On `main` it fails with the NPE above.
- `fromAndToPbFileInfo with MapFileMeta with segment granularity` is a
regression check. It confirms that recorded segment IDs and segment indexes
still round-trip.
```
build/mvn -pl common -am -Dtest=none
-DwildcardSuites=org.apache.celeborn.common.util.PbSerDeUtilsTest test
Tests: succeeded 36, failed 0
```
--
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]
