[GitHub] [spark] attilapiros commented on a change in pull request #29211: [SPARK-31197][CORE] Shutdown executor once we are done decommissioning

[GitBox <gi...@apache.org>]

attilapiros commented on a change in pull request #29211:
URL: https://github.com/apache/spark/pull/29211#discussion_r460401563



##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerDecommissioner.scala
##########
@@ -327,4 +354,28 @@ private[storage] class BlockManagerDecommissioner(
     }
     logInfo("Stopped storage decommissioner")
   }
+
+  /*
+   *  Returns the last migration time and a boolean for if all blocks have been migrated.
+   *  If there are any tasks running since that time the boolean may be incorrect.
+   */
+  private[storage] def lastMigrationInfo(): (Long, Boolean) = {
+    if (stopped || (stoppedRDD && stoppedShuffle)) {
+      (System.nanoTime(), true)
+    } else {
+      // Chose the min of the running times.
+      val lastMigrationTime = if (
+        conf.get(config.STORAGE_DECOMMISSION_SHUFFLE_BLOCKS_ENABLED) &&
+        conf.get(config.STORAGE_DECOMMISSION_RDD_BLOCKS_ENABLED)) {
+        Math.min(lastRDDMigrationTime, lastShuffleMigrationTime)

Review comment:
       I was thinking a bit more on this part. 
   
   The intention of `lastRDDMigrationTime`, `lastShuffleMigrationTime` and `lastTaskRunningTime` is to notify the driver **only once** about the exit of this executor. 
   
   My first thought was to improve this by expressing this intention directly with a simple flag in the `shutdownThread`(like `exitTriggered` which can be used as a stopping condition instead of the `while (true)` ) and remove all the three time variables and change `lastMigrationInfo` to return just a boolean and rename it for its new role (for example to `isFinished` or just simply to `finished`). 
   
   Then I went a bit further and checked the `exitExecutor` method and I think even the flag is not necessarily needed:
   https://github.com/apache/spark/blob/484f8e216d5727e516488aedbdb41b1f63569701/core/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala#L267
   
   Still I like to have a stopping condition in the while loop so please consider to use the flag instead of the times. 
         




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

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