[SPARK-4949]shutdownCallback in SparkDeploySchedulerBackend should be enclosed by synchronized block. #3781
Conversation
|
Test build #24759 has finished for PR 3781 at commit
|
|
|
||
| val registrationLock = new Object() | ||
| var registrationDone = false | ||
| private val registrationLock = new Object() |
There was a problem hiding this comment.
On the one hand, this sounds like it could be an AtomicBoolean. On another hand -- this whole mechanism could be replaced by something more robust in java.util.concurrent
sarutak
changed the title
|
Test build #24782 has finished for PR 3781 at commit
|
|
Test build #25235 has finished for PR 3781 at commit
|
|
retest this please. |
|
Test build #25244 has finished for PR 3781 at commit
|
|
This looks good to me. Most of it is making some fields private that look like they should be, simplifying one synchronization primitive, and does fix a theoretical concurrency problem. Tests pass. I will merge soon if there are no objections. |
| @volatile var appId: String = _ | ||
| private var client: AppClient = null | ||
| private var stopping = false | ||
| private val shutdownCallback: AtomicReference[(SparkDeploySchedulerBackend) => Unit] = |
There was a problem hiding this comment.
@sarutak would this not be simpler as a @volatile reference? that's how appId is handled below.
…ile reference instead of AtomicReference
|
@srowen Thank you for picking up this PR. I've changed the |
…ile reference instead of AtomicReference
|
Test build #27667 has finished for PR 3781 at commit
|
|
Test build #27668 has finished for PR 3781 at commit
|
| @@ -148,19 +152,16 @@ private[spark] class SparkDeploySchedulerBackend( | |||
| super.applicationId | |||
| } | |||
|
|
|||
| def setShutdownCallback(f: SparkDeploySchedulerBackend => Unit) { | |||
There was a problem hiding this comment.
OK, but why do you need this setter now?
There was a problem hiding this comment.
It's no longer needed so I remove it soon.
|
Test build #27678 has finished for PR 3781 at commit
|
A variable
shutdownCallbackin SparkDeploySchedulerBackend can be accessed from multiple threads so it should be enclosed by synchronized block.