[
https://issues.apache.org/jira/browse/YARN-3318?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14388275#comment-14388275
]
Craig Welch commented on YARN-3318:
-----------------------------------
Hi Wangda,
I have changed the patch a bit in the background without updating it on the
jira. The changes are not major but I think they render some of the comments
obsolete. I've uploaded the up-to-date patch just now, before doing so I took
a pass through your comments- below I'll respond to each in turn-
bq. 1) SchedulerProcess
bq. 1.1. ...name seems not very straightforward to me...
Well, I'm certainly open to other name options, but I do prefer
SchedulerProcess to SchedulableEntity - it's common to refer the items which a
scheduler will schedule as "Processes", which is what these are in this case,
which is why I chose this name. "Entity" is really very generic and empty of
meaning. I do wish to avoid confusion with Scheduleable (and wasn't enamored
of that name either...), I expect that as integration progresses there will be
a period where Schedulable will be an extension of SchedulerProcess with the
(remaining) FairScheduler specific bits (which will, I think, ultimately be
incorporated in some way into SchedulerProcess, but that's down the line/should
be addressed in further iteration). In any case, not in favor of adding
"Entity", I think when you consider the terminology as explained above
SchedulerProcess works, try it on and see, and feel free to give other
options...
bq. 1.2. ...SchedulerProcessEvent...asynchronized
Not all event handling must be asynchronous. I believe the details regarding
this were spelled out reasonably well in the interface definition - if you take
a peek at how these events are handled in the capacity scheduler configuration
you will see that they are synchronously/safely within protection against
mutation of the schedulerprocess collection. My goal was precisely to avoid
needing to have implementer add a new method implementation every time a new
event comes into play which may not be of interest to it, this makes
maintenance of implementations easier - they can manage those which they
understand and have appropriate default logic otherwise. I think this is a
classic case for an enumerated set of events to be handled by the interface &
so I think it should be modeled as it is as opposed to adding a new method for
each new event type to the interface itself...
bq. 1.3 ...SerialEpoch
Yeah, I don't like the names much either, I've gone through several versions
and come to the conclusion that it's not the choice of names that's the
problem. This is an attempt to "hide" the application id's while also exposing
them, which is compound in nature, and is made stranger by the fact that this
is totally irrelevant for other potential future implementors (such as queues).
I want to factor it differnently not just change the names, these are the
courses I'm considering:
1. Have SchedulerProcess implement "Comparable" and provide a "natural
ordering", which is fifo for apps. This seems to "privilege" fifo but, as a
matter of fact, it's the fallback for fair so I'm not sure that's really an
inappropriate thing to do - it seems like it is the "natural ordering" for
apps. Other things can give their own natural ordering (queues - the
hierarchy...), so it should extend reasonably well without the current
awkwardness. This would remove all of "getSerialEpoch", "getSerial", and
"getId" in favor of just implementing "compareTo" from comparable. The
downsides I see are the "privilage" and that if an implementor of
SchedulerProcess implemented "comparable" in an unworkable fashion it would be
an issue, not the case for what we are presently looking at supporting afaik
2. Have an explicit "compareCreationOrder(SchedulerProcess other)" method
which returns 0 + - like compareTo. This is much like 1, but removes the
"privilege" and the possible Comparable collision... this also does away
getSerialEpoch, getSerial, and getId in favor of the comparison method.
What do you think of these options? Preference?
BTW, FS has an actual "startTime" for fsappattempts, but looking through it I
don't like that approach - it doesn't appear to do the right thing in some
cases (like rm failover or recovery), it still can be ambiguous for some cases
(simultanious start w/in tsmillis granularity) where there's a fallback to
appid, so it doesn't look to really add anything as you still have to be able
to fall back to the app id for those cases so you can't get away from the
issue, and it adds a bit of complexity to boot.
bq. 1.4 ...currentConsumption is not enough to make choice,
demand(pending-resource)/used and priority/weight are basic fields of a
Schedulable, do you think so...
Of those, only demand is required for the initial step of supporting
application level fairness when sizeBasedWeight is active, the others are only
relevant for other uses (such as queues) which we're not taking on in the first
go. I had already added demand to the interface, it is presently being
calculated using pending requests. I looked at switching to using the pending
value of the attemptResourceUsage in the SchedulerApplicationAttempt but I
don't see that it is being updated anywhere, am I missing it? It seems that it
should have been made to do so when it was added to SchedulerApplicationAttempt
but that it was possibly missed? In any case, if it is/were it made to be I
could use it. Also, looking over ResourceUsage, I think there needs to be a
"getCopyPending" or the like, for usecases where you get the value and then
start to use it, as the code is today it can be mutated as soon as the value is
returned, so while in some sense it's threadsafe, not really in a usable way
for many callers who would want a snapshot copy of the final value which won't
change (as in this case...)
bq. 2) Ordering Policy
bq. 2.1 addSchedulerProcess/removeSchedulerProcess/addAllSchedulerProcesses
...Collection... treeSet
I think the current factoring is correct. First, you'll notice that the return
type of getSchedulerProcess() is a Collection, in no way is this tied to
treeSet, implementers are welcome to use any collection type they like (or
implement the minimal collection interface themselves if really necessary...).
It is appropriate to have a Collection view on the processes, it is the proper
abstraction for it and it's used in various places throughout the scheduler -
effectively the ask is to add the collection interface methods to this
interface, which doesn't make sense, it's better to use the existing/common
interface as I am doing at present
bq. 2.2 ...
no it doesn't, see 1.2 above
bq. 2.3 rename...SchedulerProcessOrderingPolicy
I suppose we could, although that assumes we keep SchedulerProcess :-) I
thought that an unnecessarily long name & that there would not be a lot of
ordering policies to give rise to ambiguity, but assuming we stick with
SchedulerProcess, or whatever we do name it, we could expand this out - not
sure its really necessary, but not really opposed...
bq. 3) ...configuration...
so, the simplified configuration you are suggesting where it's just necessary
to setup one config entry with "fair" or "fifo" is already the way it will
work. The default for the policy, if unspecified, is the
SchedulerComparatorPolicy, so for the cases we are building here, the policy
doesn't need to be set in configuration, only the comparator (if other than
fifo). I expect that most functionality will be able to be implemented via the
SchedulerComparatorPolicy (all that we are implementing at present can be...),
and so the "extra" configuration will not be needed. The higher level
abstraction is present because it provides a clear point where other approaches
could be taken, but those would not also be specifying the comparator side, and
so would also not need "double configuration". I am presently spelling out the
"FairComparator" class to ease the patch management, when the initial patch
goes in there will be a "fair" option to match the "fifo", where that is all
that would need to be specified. "fair" can actually be used on it's own
without compounding with fifo, the fall back is on the lexical appid, which
should be good for cases where "fairness" is really what is wanted, we could
add a "fairFifo" shortcut config to give a convenient way to compound them but
I don't think it's really needed. When priority becomes available I do intend
to support shortcut "priorityFair" "priorityFifo" options to allow for one-stop
compound configuration as you suggest for those cases, it's not there yet
because it can't be until priority is settled so that it can be incorporated.
(to be clear, these "shortcut" options (priorityFair, priorityFifo, others as
needed) would be in the same role as "fifo" or "compound" today, and would
configure the whole setup w/in the SchedulerComparatorPolicy)
> Create Initial OrderingPolicy Framework, integrate with CapacityScheduler
> LeafQueue supporting present behavior
> ---------------------------------------------------------------------------------------------------------------
>
> 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
>
>
> Create the initial framework required for using OrderingPolicies with
> SchedulerApplicaitonAttempts and integrate with the CapacityScheduler. This
> will include an implementation which is compatible with current FIFO behavior.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
