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-
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...
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...
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.
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...)
2) Ordering Policy
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
no it doesn't, see 1.2 above
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...
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)