Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r215750952
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -218,6 +244,8 @@ private[spark] class Executor(
env.metricsSystem.report()
heartbeater.shutdown()
heartbeater.awaitTermination(10, TimeUnit.SECONDS)
+ executorPluginThread.interrupt()
+ executorPluginThread.join()
--- End diff --
We should be careful about semantics of `interrupt()` when overriding it -
here we are using it to trigger something in addition to thread interruption.
For example, the actual shutdown of plugin's is getting triggered in this
caller thread - and not in plugin init thread : which can cause issues since
plugin's are using a a different thread ctx classloader.
This usage is actually a bit confusing.
A better option, if we want to follow current design, would be:
* Subclass Thread - expose methods to trigger state changes.
* Once init completes, wait+notify/condition wait until plugin shutdown is
required.
* On stop(), invoke plugin shutdown in plugin thread (so that right
classloader is in use).
IIRC VM shutdown results in `executor.stop` - and so plugin shutdown should
get invoked even in that case.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
