You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "s0nskar (via GitHub)" <gi...@apache.org> on 2026/04/13 07:42:48 UTC

[PR] [CELEBORN-1660][FOLLOWUP] Improving the unknown worker check [celeborn]

s0nskar opened a new pull request, #3660:
URL: https://github.com/apache/celeborn/pull/3660

   <!--
   Thanks for sending a pull request!  Here are some tips for you:
     - Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
     - Be sure to keep the PR description updated to reflect all changes.
     - Please write your PR title to summarize what this PR proposes.
     - If possible, provide a concise example to reproduce the issue for a faster review.
   -->
   
   ### What changes were proposed in this pull request?
   
   Improving the unknown worker check
   
   ### Why are the changes needed?
   
   Improving the unknown worker check, 
   
   - directly use set `removeAll` method
   - Avoid scala/java serialization
   
   ### Does this PR resolve a correctness bug?
   
   No
   
   ### Does this PR introduce _any_ user-facing change?
   
   No
   
   ### How was this patch tested?
   
   Existing UTs


-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1660][FOLLOWUP] Improving the unknown worker check [celeborn]

Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar commented on PR #3660:
URL: https://github.com/apache/celeborn/pull/3660#issuecomment-4234679729

   Never mind, i think current approach is better because needCheckedWorkerList is a list not a set.


-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1660][FOLLOWUP] Improving the unknown worker check [celeborn]

Posted by "s0nskar (via GitHub)" <gi...@apache.org>.
s0nskar closed pull request #3660: [CELEBORN-1660][FOLLOWUP] Improving the unknown worker check
URL: https://github.com/apache/celeborn/pull/3660


-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1660][FOLLOWUP] Improving the unknown worker check [celeborn]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3660:
URL: https://github.com/apache/celeborn/pull/3660#discussion_r3071566538


##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -1235,8 +1235,7 @@ private[celeborn] class Master(
       System.currentTimeMillis(),
       requestId)
     gaugeShuffleFallbackCounts()
-    val unknownWorkers = needCheckedWorkerList.asScala.filterNot(w =>
-      statusSystem.workersMap.containsKey(w.toUniqueId)).asJava
+    needCheckedWorkerList.removeAll(statusSystem.workersMap.values())

Review Comment:
   `needCheckedWorkerList.removeAll(statusSystem.workersMap.values())` can be O(M×N) because `workersMap` is a `ConcurrentHashMap` (see `AbstractMetaManager.workersMap`) and its `values()` view has linear-time `contains`, which `removeAll` relies on. Since this runs on every application heartbeat, it can become a CPU hotspot on large clusters; prefer filtering/removing based on `workersMap.containsKey(w.toUniqueId)` (e.g., `removeIf` or building a hash set of known IDs) to keep this O(M).
   ```suggestion
       needCheckedWorkerList.removeIf(worker => statusSystem.workersMap.containsKey(worker.toUniqueId))
   ```



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org