Copilot commented on code in PR #3706:
URL: https://github.com/apache/celeborn/pull/3706#discussion_r3332227554
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Controller.scala:
##########
@@ -699,18 +699,35 @@ private[deploy] class Controller(
case throwable: Throwable =>
logError(s"$errMsg, an unexpected exception occurred.",
throwable)
}
+ val response =
+ if (committedPrimaryIds.isEmpty &&
committedReplicaIds.isEmpty) {
+ CommitFilesResponse(
+ StatusCode.COMMIT_FILE_EXCEPTION,
+ List.empty.asJava,
+ List.empty.asJava,
+ primaryIds,
+ replicaIds)
+ } else {
+ CommitFilesResponse(
+ StatusCode.PARTIAL_SUCCESS,
+ new jArrayList[String](committedPrimaryIds),
+ new jArrayList[String](committedReplicaIds),
+ new jArrayList[String](failedPrimaryIds),
+ new jArrayList[String](failedReplicaIds),
+ new jHashMap[String,
StorageInfo](committedPrimaryStorageInfos),
+ new jHashMap[String,
StorageInfo](committedReplicaStorageInfos),
+ new jHashMap[String, RoaringBitmap](committedMapIdBitMap),
+ partitionSizeList.asScala.sum,
+ partitionSizeList.size())
+ }
Review Comment:
On the exceptional/timeout path, `failedPrimaryIds`/`failedReplicaIds` only
track partitions that explicitly hit `IOException` (or missing location).
Partitions that were still in-progress when the timeout cancelled the overall
future may end up in neither `committed*` nor `failed*`, and the client-side
`checkDataLost()` only considers `failed*` sets—so these uncommitted partitions
can be incorrectly treated as non-lost (effectively “empty”). Consider deriving
failed IDs as: `allIds - committedIds - emptyFileIds`, plus any
already-recorded failures, and use those lists in the response.
--
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]
