[SPARK-29152][CORE] Executor Plugin shutdown when dynamic allocation is enabled #26315
Conversation
|
All the previous comments are still outstanding. |
iRakson
force-pushed
the
Executor_Plugin
branch
from
88fe38b
to
153e879
Compare
plugin Shutdown method has been removed. executorShutdown is needed because in case of graceful shutdown of plugin , shutdown method will be called twice without this variable. |
dongjoon-hyun
changed the title
…bled: Made executorShutdown variable volatile and fixed style errors
…bled: Review Comments fix.
…bled: Move check variable to the start of method.
iRakson
force-pushed
the
Executor_Plugin
branch
from
90efbf8
to
2afcb71
Compare
|
Test build #4919 has finished for PR 26315 at commit
|
dongjoon-hyun
changed the title
| } catch { | ||
| case e: Exception => | ||
| logWarning("Plugin " + plugin.getClass().getCanonicalName() + " shutdown failed", e) | ||
| if (!executorShutdown) { |
There was a problem hiding this comment.
Although this is unlikely to be a problem, this is not thread-safe. You should use an AtomicBoolean. Or make stop() synchronized.
There was a problem hiding this comment.
executorShutdown is volatile; the race is still that it could be set between this line and the next? yeah agree. While we're here, worth fixing with AtomicBoolean and compareAndSet
|
I have made the required changes. Please re-test this one. |
| case e: Exception => | ||
| logWarning("Plugin " + plugin.getClass().getCanonicalName() + " shutdown failed", e) | ||
| if (!executorShutdown.get()) { | ||
| executorShutdown.compareAndSet(false, true) |
There was a problem hiding this comment.
This won't work to separately check and set the value; it loses the atomic-ness.
Really it just needs to be if (!executorShutdown.getAndSet(true))
|
In master, older version of executor plugin was removed today. But i feel this solution is valid for spark 2.4.4. |
|
OK, we'd need a new PR, yes |
|
@iRakson are you sure it's not applicable in master, given the comments on your other PR? |
|
i will check in master. If issue is present in master then will update here. |
What changes were proposed in this pull request?
Added a shutdown hook which will ensure that shutdown method of executor plugin is getting called.
Why are the changes needed?
When dynamic allocation is enabled and executors get killed after idle time out, shutdown method of executor plugin is not getting called.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Manual Testing