You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/08/22 10:07:08 UTC

[GitHub] [spark] boneanxs commented on a diff in pull request #27313: [SPARK-29148][CORE] Add stage level scheduling dynamic allocation and scheduler backend changes

boneanxs commented on code in PR #27313:
URL: https://github.com/apache/spark/pull/27313#discussion_r951253591


##########
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala:
##########
@@ -685,6 +835,14 @@ private[spark] class ExecutorAllocationManager(
       stageAttemptToNumRunningTask.values.sum
     }
 
+    def totalRunningTasksPerResourceProfile(rp: Int): Int = {
+      val attempts = resourceProfileIdToStageAttempt.getOrElse(rp, Set.empty).toSeq
+      // attempts is a Set, change to Seq so we keep all values
+      attempts.map { attempt =>
+        stageAttemptToNumRunningTask.getOrElseUpdate(attempt, 0)

Review Comment:
   Hi, @tgravescs here do we need to update `0` back to stageAttemptToNumRunningTask instead of using:
   
   ```scala
   stageAttemptToNumRunningTask.getOrElseUpdate(attempt, 0)
   ```
   If we update this without `allocationManager.synchronized`, it could have issues.



##########
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala:
##########
@@ -685,6 +835,14 @@ private[spark] class ExecutorAllocationManager(
       stageAttemptToNumRunningTask.values.sum
     }
 
+    def totalRunningTasksPerResourceProfile(rp: Int): Int = {
+      val attempts = resourceProfileIdToStageAttempt.getOrElse(rp, Set.empty).toSeq
+      // attempts is a Set, change to Seq so we keep all values
+      attempts.map { attempt =>
+        stageAttemptToNumRunningTask.getOrElseUpdate(attempt, 0)

Review Comment:
   Hi, @tgravescs here do we need to update `0` back to stageAttemptToNumRunningTask instead of using:
   
   ```scala
   stageAttemptToNumRunningTask.getOrElse(attempt, 0)
   ```
   If we update this without `allocationManager.synchronized`, it could have issues.



-- 
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: reviews-unsubscribe@spark.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org