SteNicholas commented on code in PR #3721:
URL: https://github.com/apache/celeborn/pull/3721#discussion_r3428276460
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Controller.scala:
##########
@@ -898,3 +921,62 @@ private[deploy] class Controller(
}
Review Comment:
Cleanup (duplication): `reply()` (line 595) and this new
`buildCommitFilesResponseOnCancel` method duplicate the response-assembly
pattern — both snapshot the concurrent
`committedPrimaryIds`/`committedReplicaIds` sets into `ArrayList`s, copy
`committedPrimaryStorageInfos`/`committedReplicaStorageInfos`/`committedMapIdBitMap`
maps via `new jHashMap(…)`, and derive `totalWritten`/`fileCount` from
`partitionSizeList`. The only differences are (a) how failed IDs are computed
(`failedIds` set vs. `primaryIds - committedIds - emptyIds`) and (b) the
status-decision logic.
If a new field is added to `CommitFilesResponse`, both must be updated in
lockstep — a missed update silently drops the field in one path. Consider
extracting the shared snapshot-and-assemble logic into a common helper, with
the differing failed-ID derivation and status logic passed in or computed by
the caller.
--
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]
