[SPARK-8059] [yarn] Wake up allocation thread when new requests arrive. #6600
Conversation
This should help reduce latency for new executor allocations.
|
Test build #34042 has finished for PR 6600 at commit
|
|
LGTM. I was trying to think if there was anything in |
| @@ -359,7 +360,9 @@ private[spark] class ApplicationMaster( | |||
| } | |||
| logDebug(s"Number of pending allocations is $numPendingAllocate. " + | |||
| s"Sleeping for $sleepInterval.") | |||
| Thread.sleep(sleepInterval) | |||
| allocatorLock.synchronized { | |||
| allocatorLock.wait(sleepInterval) | |||
There was a problem hiding this comment.
In this case it might be ok, but a wait call should not be called from outside of a while: http://stackoverflow.com/questions/1038007/why-should-wait-always-be-called-inside-a-loop
(in this case too, we'd still want to protect against spurious wake ups - so adding a loop is good)
There was a problem hiding this comment.
Yeah, I think it's not strictly necessary but good practice. Though here we're technically in a loop, just a bigger one. Since we intend to allocate stuff periodically the worst thing that can possibly happen is some additional latency, and this only happens if there is a 3rd thread somewhere (does not exist).
In other words the spurious wake ups are benign and not possible given the changes here. I'm fine with this being merged as is.
|
Yeah spurious wakeup doesn't matter here. It would just check again early. |
| */ | ||
| def requestTotalExecutors(requestedTotal: Int): Unit = synchronized { | ||
| def requestTotalExecutors(requestedTotal: Int): Boolean = synchronized { |
There was a problem hiding this comment.
The only slightly scary thing is that we have another method with the exact same signature, but the return value means something different. Since we document this clearly it should be fine.
|
LGTM. I'll let this sit for another day or so in case others have anything else to add. |
|
Yep, in this case it is likely fine, but is good practice to have the loop On Wednesday, June 3, 2015, andrewor14 [email protected] wrote:
Thanks, |
|
In case you guys have not noticed, there's no variable to check, so you can't loop on anything. This is just a plain "wake up". Yes, you could add one, but that would just complicate the code (e.g. figuring out how much to sleep after a spurious wake up). |
|
It's good practice when you are |
|
Not suggesting that we change it here - but it looks like ErrorProne does have a check which we might have to disable for this file - http://errorprone.info/bugpattern/WaitNotInLoop |
|
Merging into master. @vanzin Is this worth back porting into 1.4? I didn't because the original patch that reduces heartbeat latency is not in there. |
This should help reduce latency for new executor allocations. Author: Marcelo Vanzin <[email protected]> Closes apache#6600 from vanzin/SPARK-8059 and squashes the following commits: 8387a3a [Marcelo Vanzin] [SPARK-8059] [yarn] Wake up allocation thread when new requests arrive.
This should help reduce latency for new executor allocations. Author: Marcelo Vanzin <[email protected]> Closes apache#6600 from vanzin/SPARK-8059 and squashes the following commits: 8387a3a [Marcelo Vanzin] [SPARK-8059] [yarn] Wake up allocation thread when new requests arrive.
This should help reduce latency for new executor allocations.