[ https://issues.apache.org/jira/browse/YARN-7612?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16295858#comment-16295858 ]
Konstantinos Karanasos commented on YARN-7612: ---------------------------------------------- I looked at the latest patch and had an offline discussion with [~asuresh]. First, we agreed to split the current JIRA in three parts to better review it: * API of the processor framework * Implementation of the processor framework * Coupling with Capacity Scheduler Some first comments in the meantime: * I know it might not be that easy, but let's try to remove the isConstraintedAllocation from the RMContainer. It will simplify a lot the code of the CapacityScheduler adn the FicaSchedulerApp. * One downside of the current implementation is that it relies on the commit API that is there only in the CapacityScheduler... This is not a blocker for now, but we should see in another JIRA what it will take to make this work for the Fair Scheduler too. * Do we need the placeApplication in the RMAppImpl? * CapacityScheduler: ** Can we unify the two createResourceCommitRequest in the CapacityScheduler that seem to duplicate a lot of code? ** In the createResourceCommitRequest, since it assumes we request a single container in the SchedulingRequest, shouldn't we add a check for that? ** The SchedulerContainer is confusing... Looks like an inner class to me, for sure it deserves a better name. * Does the NodeCandidateSelector belong to the constraint/processor package? * ApplicationMasterService: since biggest part of the serviceInit is now the AMS-processor-chain related stuff, let's put all that initialization in a separate amsProcessorInit method. Also some minor comments: * YarnConfiguration: placement.algorithm -> constraint-placement.algorithm * yarn_protos.proto: ** RR prefix reminds me of ResourceRequest, instead of RejectionReason. Same in ProtoUtils. Maybe we can do sth like PRR (PlacementRR). ** COULD_NOT_SCHEDULE_ON_PLACED_NODE -> COULD_NOT_SCHEDULE_ON_NODE * In the ApplicationMasterServiceUtils, I would put the setRejectedSchedulingRequests inside the first if clause, assuming most responses will not have a rejection. * Typo in RMActiveServiceContext, RMContext, and RMContextImpl: PlacementConstriantsManager * RMContainerImpl: isConstraintAllocation, is ConstraintPlacement… Make consistent * ResourceScheduler: ** True is -> true if ** tryAllocate -> attemptAllocation? Or better attemptAllocationOnNode? ** "Propose a SchedulerRequest for the Scheduler to try allocate" -> "attempt to place a SchedulerRequest" * constraint/api/Algorithm: rename to sth like PlacementAlgorithm > Add Placement Processor Framework > --------------------------------- > > Key: YARN-7612 > URL: https://issues.apache.org/jira/browse/YARN-7612 > Project: Hadoop YARN > Issue Type: Sub-task > Reporter: Arun Suresh > Assignee: Arun Suresh > Attachments: YARN-7612-YARN-6592.001.patch, > YARN-7612-YARN-6592.002.patch, YARN-7612-YARN-6592.003.patch, > YARN-7612-YARN-6592.004.patch, YARN-7612-YARN-6592.005.patch, > YARN-7612-YARN-6592.006.patch, YARN-7612-v2.wip.patch, YARN-7612.wip.patch > > > This introduces a Placement Processor and a Planning algorithm framework to > handle placement constraints and scheduling requests from an app and places > them on nodes. > The actual planning algorithm(s) will be handled in a YARN-7613. -- This message was sent by Atlassian JIRA (v6.4.14#64029) --------------------------------------------------------------------- To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org