[ 
https://issues.apache.org/jira/browse/YARN-2080?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14125994#comment-14125994
 ] 

Vinod Kumar Vavilapalli commented on YARN-2080:
-----------------------------------------------

Some comments on the patch:
 - Configuration
    -- admission.enable -> Rename to reservations.enable?
    -- RM_SCHEDULER_ENABLE_RESERVATIONS -> RM_RESERVATIONS_ENABLE, 
DEFAULT_RM_SCHEDULER_ENABLE_RESERVATIONS -> DEFAULT_RM_RESERVATIONS_ENABLE
    -- reservation.planfollower.time-step -> 
reservation-system.plan-follower.time-step
    -- RM_PLANFOLLOWER_TIME_STEP, DEFAULT_RM_PLANFOLLOWER_TIME_STEP -> 
RM_RESERVATION_SYSTEM_PLAN_FOLLOWER_TIME_STEP, 
DEFAULT_RM_RESERVATION_SYSTEM_PLAN_FOLLOWER_TIME_STEP
- A meta question about configuration: It seems like if I pick up a scheduler 
and enable reservations, the system-class, the plan-follower should be picked 
up automatically instead of them being standalone configs. Can we do that? 
Otherwise the following
    -- reservation.class -> reservation-system.class? 
    -- RM_RESERVATION, DEFAULT_RM_RESERVATION -> RM_RESERVATION_SYSTEM_CLASS, 
DEFAULT_RM_RESERVATION_SYSTEM_CLASS 
    -- reservation.plan.follower -> reservation-system.plan-follower
    -- RM_RESERVATION_PLAN_FOLLOWER, DEFAULT_RM_RESERVATION_PLAN_FOLLOWER -> 
RM_RESERVATION_SYSTEM_PLAN_FOLLOWER, DEFAULT_RM_RESERVATION_SYSTEM_PLAN_FOLLOWER
 - YarnClient.submitReservation(): We don't return a queue-name anymore after 
the latest YARN-1708? There are javadoc refs to the queue-name being returned.
 - ClientRMService
    -- If reservations are not enabled, we get a host of "Reservation is not 
enabled. Please enable & try again" everytime which is not desirable. See 
checkReservationSystem(). This log and a bunch of similar logs in 
ReservationInputValidator may either be (1) deleted or (2) actually belong to 
the audit-log (RMAuditLogger) - we don't need to double-log
    -- checkReservationACLs: Today anyone who can submit applications can also 
submit reservations. We may want to separate them, if you agree, I'll file a 
ticket for future separation of these ACLs.
 - AbstractReservationSystem
    -- getPlanFollower() -> createPlanFollower()
    -- create and init plan-follower should be in serviceInit()?
    -- getNewReservationId(): Use ReservationId.newInstance()
 - ReservationInputValidator: Deleting a request shouldn't need 
validateReservationUpdateRequest->validateReservationDefinition. We only need 
the ID validation.
 - CapacitySchedulerConfiguration: I don't understand the semantics of configs 
- average-capacity, reservable.queue, reservation-window, 
reservation-enforcement-window, instantaneous-max-capacity,  - yet as they are 
not used in this patch. Can we drop them (and their setters/getters) here and 
move them to the JIRA that actually uses them?

Tests
 - TestYarnClient: You can use the newInstance methods and avoid using pb 
implementations and the setters directly (for e.g {{new 
ReservationDeleteRequestPBImpl()}}
 - TestClientRMService:
    -- ReservationRequest.setLeaseDuration() was renamed to be simply 
setDuration() in YARN-1708. Seems like there are other such occurrences in the 
patch.
    -- Similary to TestYarnClient, use record.newInstance methods instead of 
directly invoking PBImpls.

Can't understand CapacityReservationSystem yet as I have to dig into the 
details of YARN-1709.

> Admission Control: Integrate Reservation subsystem with ResourceManager
> -----------------------------------------------------------------------
>
>                 Key: YARN-2080
>                 URL: https://issues.apache.org/jira/browse/YARN-2080
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: resourcemanager
>            Reporter: Subramaniam Krishnan
>            Assignee: Subramaniam Krishnan
>         Attachments: YARN-2080.patch, YARN-2080.patch, YARN-2080.patch
>
>
> This JIRA tracks the integration of Reservation subsystem data structures 
> introduced in YARN-1709 with the YARN RM. This is essentially end2end wiring 
> of YARN-1051.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to