Skip to content

[MINOR] Fix NPE when persisting MapFileMeta without segment granularity - #3874

Open
attiasr wants to merge 1 commit into
apache:mainfrom
attiasr:fix-map-fileinfo-serde
Open

attiasr wants to merge 1 commit into
apache:mainfrom
attiasr:fix-map-fileinfo-serde

Conversation

@attiasr

@attiasr attiasr commented Oct 14, 2026

Copy link
Copy Markdown

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?

  • 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

MapFileMeta only creates its writing-segment map and segment indexes on the
first segment, so toPbFileInfo threw an NPE for MAP partitions written without
segment granularity. The worker's committed file info saver then stopped and the
shutdown save skipped those shuffles, so they could not be recovered after a
graceful restart.
// Both are created lazily on the first segment: null without segment granularity.
if (mapFileMeta.getSubPartitionWritingSegmentId != null) {
builder.putAllPartitionWritingSegment(mapFileMeta.getSubPartitionWritingSegmentId)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@yew1eb yew1eb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1, LGTM. Verified against main: both fields are lazily initialized in MapFileMeta, and the worker builds MAP file metas via the no-arg constructor in both createMemoryFileInfo and getFileMeta, so both stay null without segment granularity. The restore path is also safe — restored metas are writer-closed and non-segment MAP partitions are read through plain MapPartitionData.

One process nit: the PR title is missing a JIRA id — Apache projects generally expect [CELEBORN-xxxx] in the title. Could you file a JIRA and retitle?

This branch has not been deployed

No deployments
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.

2 participants