[
https://issues.apache.org/jira/browse/YARN-3318?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14395508#comment-14395508
]
Craig Welch commented on YARN-3318:
-----------------------------------
[~vinodkv]
bq. ...We can strictly focus on the policy framework here...
Sure, limited patch to "framework"
bq. ...You could also say SchedulableProcess...
SchedulableProcess it is, done
bq. I agree to this, but we are not in a position to support the APIs, CLI,
config names in a supportable manner yet. They may or may not change depending
on how parent queue policies, limit policies evolve. For that reason alone, I
am saying that (1) Don't make the configurations public yet, or put a warning
saying that they are unstable and (2) don't expose them in CLI , REST APIs yet.
It's okay to put in the web UI, web UI scraping is not a contract.
You can't see it, because it's part of "Capacity Scheduler Integration", but
removed CLI and proto related change. There was no rest api change, the web UI
change is still present. Will warn unstable when added to config files in the
scheduler integration patch
bq. SchedulerApplicationAttempt.getDemand() should be private
Done
bq. updateCaches() -> updateState() / updateSchedulingState() as that is what
it is doing? getCachedConsumption() / getCachedDemand(): simply getCurrent*()
? What is the need for reorderOnContainerAllocate () /
reorderOnContainerRelease()?
Is now getSchedulingConsumption(); getSchedulingDemand();
updateSchedulingState();
This is needed because mutable values which are used for ordering cannot be
allowed to change for an item in the tree, else it will not be found in some
cases during the delete before reinsert process which occurs when a
schedulable's mutable values used in comparison change (for fairness, changes
to consumption and potentially demand) Not all OrderingPolicies require
reordering on these events, for efficiency they get to decide if they do or
not, hence the "reorderOn". The "reorderOn" are now
reorderForContainerAllocation reorderForContainerRelease
bq. Move all the comparator related classed into their own package
No longer needed as comparators are now just a property of policies, see below
for details
bq. This is really a ComparatorBasedOrderingPolicy. Do we really see
non-comparator based ordering-policy. We are unnecessarily adding two
abstractions - adding policies and comparators
Originally, there was a perceived need to be able to support a more flexible
interface than the comparator one, but also a desire to build up a simpler,
composible abstraction to be used with an instance of the former which had
"most of the hard stuff done". Given that all of the policies we've
contemplated building fit the latter abstraction and the level of flexibility
does not appear to actually be that different, I think it's fair to say that we
only need what was previously the "SchedulerComparator" abstraction as a
plugin-point. Given that, a slightly refactored version of the
"SchedulerComparator" abstraction is now the only plugin point and is now what
goes by the name of "OrderingPolicy". What was previously the "OrderingPolicy"
is now a single concrete class implementing the surrounding logic, meant to be
usable from any scheduler, named "SchedulingOrder". So, one abstraction, a
comparator-based ordering-policy. If we really do find we need a flexibility
we don't have some day, the SchedulingOrder class could be abstracted to
provide that higher level abstraction - but as we see no need for it now, and
it appears probably never will, there's no reason to do so at present
bq. ...Use className.getName()...
Done
[~leftnoteasy]
bq. ...I prefer what Vinod suggested, split "SchedulerProcess" to be
"QueueSchedulable" and "AppSchedulable" ...
I don't see that he has suggested that. In any case, with the removal of
"*Serial*" and the move to compareInputOrderTo() I don't at present see a need
to have separate subtypes for app and queue to avoid "dangling properties".
And, I think if we do it right we won't end up introducing them. By splitting
in the suggested way we commit ourselves to either multiple comparators (to use
the differing functionality) or awkward testing of subtype/etc logic in one
comparator - so it basically moves the complexity/awkwardness, it doesn't
eliminate it. I've refactored such that the Policy now provides a Comparator
as opposed to extending it, so there is now room for it to provide multiple
comparators and handle subtypes if need be, but I think we should wait until we
see that we must do that before doing so, as I don't believe we will end up
needing to (but if we do, existing code should need little change, and
implementing what you suggest should be essentially additive...)
bq. ...About inherit relationships between interfaces/classes...
Policies will be composed to achieve combined capabilities yet the collection
of SchedulableProcesses & as such must reside in one instance, so attempting to
place that logic in the multiple-instances (where multiple policies are
constructed, each containing a SchedulerProcess collection via inheritance) is
a non starter. Before the latest refactor this was handled by having a single
SchedulerComparatorOrderingPolicy with multiple Comparators, post the latest
refactor the SchedulingOrder is a single concrete type containing the
SchedulableProcess collection, and using one or more OrderingPolicies which
possess the policy specific logic (including, but not limited to, comparator
logic). As it happens, they do not appear to need to share logic, so there is
no super class, just a shared interface implementation - were that to change we
could certainly adjust the implementation, but there's no need to now, and it's
an implementation not an api issue anyway.
bq. ...Regarding relationship between OrderingPolicy and comparator...
There's no longer this duality, there are now just OrderingPolicies
bq. ...Regarding configuration...
There's now just one type of configuration, for policy, and will handle
"friendly policy labels". Only the fifo is available now, and generalized
composition logic, it's in SchedulingOrder
> Create Initial OrderingPolicy Framework and FifoOrderingPolicy
> --------------------------------------------------------------
>
> Key: YARN-3318
> URL: https://issues.apache.org/jira/browse/YARN-3318
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: scheduler
> Reporter: Craig Welch
> Assignee: Craig Welch
> Attachments: YARN-3318.13.patch, YARN-3318.14.patch,
> YARN-3318.17.patch, YARN-3318.34.patch, YARN-3318.35.patch,
> YARN-3318.36.patch, YARN-3318.39.patch, YARN-3318.45.patch, YARN-3318.47.patch
>
>
> Create the initial framework required for using OrderingPolicies and an
> initial FifoOrderingPolicy
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
