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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
