[GitHub] [incubator-celeborn] RexXiong commented on a diff in pull request #1594: [CELEBORN-682] Master and client should handle blacklist worker and shutting down worker separately

["RexXiong (via GitHub)" <gi...@apache.org>]

RexXiong commented on code in PR #1594:
URL: https://github.com/apache/incubator-celeborn/pull/1594#discussion_r1230388971


##########
client/src/main/scala/org/apache/celeborn/client/ChangePartitionManager.scala:
##########
@@ -253,6 +253,7 @@ class ChangePartitionManager(
         .keySet()
         .asScala
         .filter(w => !lifecycleManager.workerStatusTracker.blacklist.keySet().contains(w))

Review Comment:
   It's nice if encapsulate these filter logic into workerStatusTracker



##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -693,6 +693,9 @@ class LifecycleManager(appId: String, val conf: CelebornConf) extends RpcEndpoin
           failureInfos.add(s"[reserveSlots] Failed to" +
             s" reserve buffers for ${Utils.makeShuffleKey(applicationId, shuffleId)}" +
             s" from worker ${workerInfo.readableAddress()}. Reason: ${res.reason}")
+          if (res.status.equals(StatusCode.WORKER_SHUTDOWN)) {
+            workerStatusTracker.resolveShutdownWorkers(List(workerInfo).asJava)

Review Comment:
   Do we need notify the listeners this time？



##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -736,7 +737,7 @@ private[celeborn] class Master(
     context.reply(CheckQuotaResponse(isAvailable, reason))
   }
 
-  private def workersNotBlacklisted(
+  private def workersAvailable(

Review Comment:
   Probably workersAvailable need consider shutdown workers every time? so filter with shutdownWorkerSnapshot together with blacklist, then param  may for other Scenario.



-- 
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