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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
