[SPARK-3456] YarnAllocator on alpha can lose container requests to RM #2373
Conversation
|
cc @vanzin |
|
QA tests have started for PR 2373 at commit
|
|
LGTM. Guess I assumed alpha would work like stable in this area... |
|
yeah no big deal, I should have seen this in testing it originally. Thanks for the review. I'll wait for jenkins on this. |
|
QA tests have finished for PR 2373 at commit
|
| @@ -121,7 +124,7 @@ private[yarn] abstract class YarnAllocator( | |||
| logDebug("Empty allocation request ...") | |||
| } | |||
|
|
|||
| val allocateResponse = allocateContainers(missing) | |||
| val allocateResponse = allocateContainers(missing, executorsPending) | |||
There was a problem hiding this comment.
If we expose the number of requests pending to the subclass, we don't need to make this take an extra parameter. E.g.
protected def numContainersPending = numPendingAllocate.get()
Then the alpha YarnAllocationHandler#allocateContainers can add this to count instead of the new parameter.
There was a problem hiding this comment.
True but it seems kind of weird to have it pass count but then grab a global for pending. It would also be easier to unit test taking it as paramater (if we had unit tests for this)
There was a problem hiding this comment.
I just thought it would simplify the method signatures. Right now the stable code takes in a parameter that it doesn't use, and in the alpha code all we do with those two parameters is add them together anyway. The number of pending container requests is a property of the allocator, so I think it makes sense if all methods in the class have access to this. (Not a huge deal)
There was a problem hiding this comment.
I understand the concern over the interfaces, but in the end hopefully this doesn't matter as hopefully we will be removing alpha support in another release. Also its an internal private interface.
To me this is safer to have one unused variable vs exposing a private variable. For instance, you will notice that if I removed all changes to YarnAllocator (except for making numPendingAllocate protected) and just grabbed it from the alpha/YarnAllocateHandler, it would be wrong - it would ask for 2x the number because we increment the numberPendig before calling allocateContainers. To me exposing that variable could more easily lead to issues like that and perhaps others accessing it and leading to more complicated usage numPending in the future. It also seems more brittle if someone comes in here and refactors things again.
I could change YarnAllocator to rearrange it to call allocateContainers before incrementing numPending for now and put comment there saying the order matters.
I would prefer to leave it, but If you would really like me to I will.
There was a problem hiding this comment.
I think it's safe to expose the variable as long as it does what its name suggests. We actually already do this elsewhere in the same class for similar variables (e.g. getNumExecutorRunning or getNumExecutorFailed). In our case, we can't make just those changes because we increment numPendingAllocate before we allocateContainers, but we can easily work around that as you mentioned. These allocation requests aren't technically pending until we've submitted them, so I think it's actually more correct to increment numPendingAllocate afterwards.
Anyway, just a minor suggestion since this is an internal API. It's fine to leave it as is if you prefer.
No description provided.