yew1eb commented on code in PR #3881:
URL: https://github.com/apache/celeborn/pull/3881#discussion_r4313249048
##########
client/src/main/scala/org/apache/celeborn/client/commit/ReducePartitionCommitHandler.scala:
##########
@@ -154,7 +154,8 @@ class ReducePartitionCommitHandler(
if (allocatedWorkers != null) {
return workerStatusTracker.excludedWorkers.asScala.collect {
case (workerId, (status, _))
- if status == StatusCode.WORKER_UNKNOWN &&
allocatedWorkers.contains(workerId) =>
+ if status == StatusCode.WORKER_UNKNOWN &&
+ allocatedWorkers.containsKey(workerId.toUniqueId) =>
Review Comment:
Verified locally: `allocatedWorkers` is a `ConcurrentHashMap[String,
ShufflePartitionLocationInfo]` whose keys are written as
`workerInfo.toUniqueId` (see `LifecycleManager#handleRegisterShuffleResponse`).
`ConcurrentHashMap.contains(Object)` is the legacy Hashtable-style method that
checks **values**, and since the values are `ShufflePartitionLocationInfo`
(never equal to a `WorkerInfo`), the old guard was always `false` — the early
`SHUFFLE_DATA_LOST` path introduced by CELEBORN-2166 has been dead code since
then.
`containsKey(workerId.toUniqueId)` matches the key format exactly and is
consistent with the lookups in `WorkerStatusTracker#excludeWorkerFromPartition`
and `CommitManager#handleCommitFiles`. LGTM on the fix itself.
Optional nit: since we're touching this expression anyway, `collect { ...
}.nonEmpty` could be simplified to `exists { case (workerId, (status, _)) =>
... }` to avoid building an intermediate collection for a membership check.
--
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]
