You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "attiasr (via GitHub)" <gi...@apache.org> on 2026/10/14 16:48:17 UTC
[PR] [MINOR] Fix NPE when persisting MapFileMeta without segment granularity [celeborn]
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2481] Fix NPE when persisting MapFileMeta without segment granularity [celeborn]
Posted by "attiasr (via GitHub)" <gi...@apache.org>.
attiasr commented on PR #3874:
URL: https://github.com/apache/celeborn/pull/3874#issuecomment-6227908311
that's for the review @yew1eb.
I've opened a ticket and renamed the PR
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2481] Fix NPE when persisting MapFileMeta without segment granularity [celeborn]
Posted by "zaynt4606 (via GitHub)" <gi...@apache.org>.
zaynt4606 commented on PR #3874:
URL: https://github.com/apache/celeborn/pull/3874#issuecomment-6259429918
**Merge Summary:**
- merged into main https://github.com/apache/celeborn/commit/bc06a183
- merged into branch-0.7 https://github.com/apache/celeborn/commit/6b0c0b02
*Posted by `merge_pr.py`*
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MINOR] Fix NPE when persisting MapFileMeta without segment granularity [celeborn]
Posted by "yew1eb (via GitHub)" <gi...@apache.org>.
yew1eb commented on code in PR #3874:
URL: https://github.com/apache/celeborn/pull/3874#discussion_r4278646302
##########
common/src/main/scala/org/apache/celeborn/common/util/PbSerDeUtils.scala:
##########
@@ -161,9 +161,14 @@ object PbSerDeUtils {
builder.setBufferSize(mapFileMeta.getBufferSize)
builder.setNumSubpartitions(mapFileMeta.getNumSubpartitions)
builder.setIsSegmentGranularityVisible(mapFileMeta.isSegmentGranularityVisible)
- builder.putAllPartitionWritingSegment(mapFileMeta.getSubPartitionWritingSegmentId)
- builder.addAllSegmentIndex(
- mapFileMeta.getSubPartitionSegmentIndexes.asScala.map(toPbSegmentIndex).asJava)
+ // Both are created lazily on the first segment: null without segment granularity.
+ if (mapFileMeta.getSubPartitionWritingSegmentId != null) {
+ builder.putAllPartitionWritingSegment(mapFileMeta.getSubPartitionWritingSegmentId)
+ }
Review Comment:
Verified the NPE and that the periodic saver (`saveCommittedFileInfosExecutor`) silently stops after the first uncaught exception. LGTM.
One note: after restore these fields are empty collections rather than null, so `MapFileMeta#hasPartitionSegmentIds()` now returns `true` for restored non-segment MAP partitions. That is harmless today since reader selection is based on `isSegmentGranularityVisible()`, but it is a subtle semantic difference worth a sentence in the PR description.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2481] Fix NPE when persisting MapFileMeta without segment granularity [celeborn]
Posted by "attiasr (via GitHub)" <gi...@apache.org>.
attiasr commented on code in PR #3874:
URL: https://github.com/apache/celeborn/pull/3874#discussion_r4302546582
##########
common/src/main/scala/org/apache/celeborn/common/util/PbSerDeUtils.scala:
##########
@@ -161,9 +161,14 @@ object PbSerDeUtils {
builder.setBufferSize(mapFileMeta.getBufferSize)
builder.setNumSubpartitions(mapFileMeta.getNumSubpartitions)
builder.setIsSegmentGranularityVisible(mapFileMeta.isSegmentGranularityVisible)
- builder.putAllPartitionWritingSegment(mapFileMeta.getSubPartitionWritingSegmentId)
- builder.addAllSegmentIndex(
- mapFileMeta.getSubPartitionSegmentIndexes.asScala.map(toPbSegmentIndex).asJava)
+ // Both are created lazily on the first segment: null without segment granularity.
+ if (mapFileMeta.getSubPartitionWritingSegmentId != null) {
+ builder.putAllPartitionWritingSegment(mapFileMeta.getSubPartitionWritingSegmentId)
+ }
Review Comment:
Good point, thanks. Rather than documenting the difference, I made `fromPbFileInfo` restore empty segment fields as `null` (4c2784e9d), so `hasPartitionSegmentIds()` on a restored non-segment MAP meta matches a live one. The round-trip tests now assert it in both directions: `false` without segment granularity, `true` with it.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2481] Fix NPE when persisting MapFileMeta without segment granularity [celeborn]
Posted by "zaynt4606 (via GitHub)" <gi...@apache.org>.
zaynt4606 commented on PR #3874:
URL: https://github.com/apache/celeborn/pull/3874#issuecomment-6259441227
Thanks @attiasr for the contribution!
Merged to main(v1.0.0), branch-0.7(v0.7.1).
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2481] Fix NPE when persisting MapFileMeta without segment granularity [celeborn]
Posted by "zaynt4606 (via GitHub)" <gi...@apache.org>.
zaynt4606 closed pull request #3874: [CELEBORN-2481] Fix NPE when persisting MapFileMeta without segment granularity
URL: https://github.com/apache/celeborn/pull/3874
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org