Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21390#discussion_r189813156
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
---
@@ -732,6 +736,9 @@ private[deploy] class Worker(
trimFinishedExecutorsIfNecessary()
coresUsed -= executor.cores
memoryUsed -= executor.memory
+ if (CLEANUP_NON_SHUFFLE_FILES_ENABLED) {
+
shuffleService.executorRemoved(executorStateChanged.execId.toString, appId)
--- End diff --
Re-reading this code with fresh eyes made me realize that maybe the method
naming here is a bit confusing: from the name alone, it sounds like
`executorRemoved` is a notification-sending method, the kind of thing where
you'd always want to call it when the corresponding event (executor removal)
occurs and then leave it up to the method implementation to decide whether to
actually carry out specific cleanup logic.
On the other hand, we call `shuffleService.applicationRemoved()` in
`maybeCleanupApplication()` and there's a flag in that method controlling
whether we actually clean up, so we have a similar pattern there.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
