Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/2373#discussion_r17507134
--- Diff:
yarn/common/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala ---
@@ -121,7 +124,7 @@ private[yarn] abstract class YarnAllocator(
logDebug("Empty allocation request ...")
}
- val allocateResponse = allocateContainers(missing)
+ val allocateResponse = allocateContainers(missing, executorsPending)
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
