Jian He commented on YARN-1712:

Thanks Subra and Carlo working on the patch. Some comments and questions on the 

- I think the default queue can be initialized upfront when PlanQueue is 
initialized in CapacityScheduler
      // Add default queue if it doesnt exist
      if (scheduler.getQueue(defPlanQName) == null) {
- Consolidate the comments into 2 lines
      // identify the reservations that have expired and new reservations that
      // have to
      // be activated
- Exceptions like the following are ignored. Is this intentional ?
     } catch (YarnException e) {
              "Exception while trying to release default queue capacity for 
plan: {}",
              planQueueName, e);
- may be create a common method to calculate lhsRes and rhsRes
      CSQueue lhsQueue = scheduler.getQueue(lhs.getReservationId().toString());
      if (lhsQueue != null) {
        lhsRes =
      } else {
        lhsRes = lhs.getResourcesAtTime(now);
- allocatedCapacity, may rename to reservedResources
      Resource allocatedCapacity = Resource.newInstance(0, 0);
- Instead of doing the following:  
      for (CSQueue resQueue : resQueues) {
      Set<String> expired =
          Sets.difference(previousReservations, curReservationNames);
      Set<String> toAdd =
          Sets.difference(curReservationNames, previousReservations);
we can do something like this to save some time cost. 
for queue in previousReservations:
    if (queue not in curReservationNames)
       curReservationNames.remove(queue) // curReservationNames contains the 
ToAdd queues in the end
- Not sure if this method is only used by PlanFollower. If it is, we can change 
the return value to be a set of reservation names so that we don’t need to loop 
later to get all the reservation names..
      Set<ReservationAllocation> currentReservations =
- rename defPlanQName to defReservationQueue
      String defPlanQName = planQueueName + PlanQueue.DEFAULT_QUEUE_SUFFIX;
- The apps are already in current planQueue, IIUC, this is the 
defaultReservationQueue ? If that, I think we may change the queueName 
parameter to the proper defaultReservationQueue name. Also, 
AbstractYarnScheduler#moveAllApps is actually expecting the queue to be 
leafQueue(ReservationQueue), not planQueue(parentQueue).
// Move all the apps in these queues to the PlanQueue
moveAppsInQueues(toMove, planQueueName);
- I’m thinking if we can make PlanFollower synchronously move apps to the 
defaultQueue, for the following reasons:
1. IIUC, the logic for moveAll and killAll is that: the first Time 
synchronizePlan is called, it will try to move all expired apps; next Time 
synchronizePlan is called, it will kill all the previously not-yet-moved apps. 
If the synchronizePlan interval is very small,  it’s likely to kill most apps 
that are being move.
2. Exceptions from CapacityScheduler#moveApplication are currently just 
ignored, if doing asynchronously 
3. PlanFollower is now anyways locking the whole scheduler in synchronizePlan 
method (though I’m still thinking if we need to lock the whole scheduler as 
this is kind of costly.)
4. In AbstractYarnScheduler#moveAllApps, We can do the moveApp synchronously, 
and still send events to RMApp to update its bookkeeping if needed. (But I 
don’t think we need to send the event now.)
5. PlanFollower move logic should be much simpler if doing synchronously 

> Admission Control: plan follower
> --------------------------------
>                 Key: YARN-1712
>                 URL: https://issues.apache.org/jira/browse/YARN-1712
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: capacityscheduler, resourcemanager
>            Reporter: Carlo Curino
>            Assignee: Carlo Curino
>              Labels: reservations, scheduler
>         Attachments: YARN-1712.1.patch, YARN-1712.2.patch, YARN-1712.patch
> This JIRA tracks a thread that continuously propagates the current state of 
> an inventory subsystem to the scheduler. As the inventory subsystem store the 
> "plan" of how the resources should be subdivided, the work we propose in this 
> JIRA realizes such plan by dynamically instructing the CapacityScheduler to 
> add/remove/resize queues to follow the plan.

This message was sent by Atlassian JIRA

Reply via email to