Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-824

Support a hierarchy of queues in the capacity scheduler

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: capacity-sched
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Support hierarchical queues in the CapacityScheduler to allow for more predictable sharing of cluster resources.

      Description

      Currently in Capacity Scheduler, cluster capacity is divided among the queues based on the queue capacity. These queues typically represent an organization and the capacity of the queue represents the capacity the organization is entitled to. Most organizations are large and need to divide their capacity among sub-organizations they have. Or they may want to divide the capacity based on a category or type of jobs they run. This JIRA covers the requirements and other details to provide the above feature.

      1. MAPREDUCE-824-7.patch
        329 kB
        rahul k singh
      2. MAPREDUCE-824-6.patch
        336 kB
        Hemanth Yamijala
      3. HADOOP-824-5.patch
        337 kB
        rahul k singh
      4. HADOOP-824-4.patch
        337 kB
        rahul k singh
      5. HADOOP-824-3.patch
        277 kB
        rahul k singh
      6. HADOOP-824-2.patch
        181 kB
        rahul k singh
      7. HADOOP-824-1.patch
        141 kB
        rahul k singh

        Activity

        Hide
        Hemanth Yamijala added a comment -

        Here is a proposal that came out after some discussions for consideration:

        User Scenario

        Consider an organization "Org1". Suppose they have a queue which is allocated a capacity of 60% of the cluster resources. Org1 has the following types of jobs:

        • Production jobs
        • Research jobs - belonging to three projects - say Proj1, Proj2 and Proj3
        • Miscellaneous types of jobs.

        Org1 would like to have a greater control over how the 60% capacity is used effectively amongst these types of jobs. For e.g. they'd like the research jobs and miscellaneous jobs to get some minimum amount of capacity for their work, and a significant share of the capacity to be alloted to production jobs. Production jobs are submitted somewhat rarely to the cluster. Whenever they are submitted, they must get the first chance to run, followed by research jobs and then miscellaneous jobs. However, whenever there are no production jobs, the research jobs must get this unused capacity.

        A Sample Configuration

        The following is an illustrative expression of the features/changes we plan to implement in the capacity scheduler to solve the use cases mentioned in the User Scenario. The actual specification may be different from the one described here. However, effort will be made to keep it as close to this expression as possible.

        grid {
          Org1 min=60% {
            priority min=90% {
              production min=82%
              proj1 min=6% max=10%
              proj2 min=6%
              proj3 min=6%
            }
            miscellaneous min=10%
          }
          Org2 min=40%
        }
        

        Description

        The features introduced in the sample configuration are described below.

        Sub-queues

        • Sub-queues are queues configured within queues. They provide a mechanism for administrators to link logically related queues (say on the basis of the organization they are set up for).
        • Using sub-queues, administrators would primarily be able to use capacity allocated to their organization more effectively than in the current model.
        • For instance, in the current implementation which has only a single level of queues, if there is unused capacity in a queue, it is divided among remaining queues in proportion to their capacities. This has the disadvantage that logically unrelated queues could stake a claim to this unused capacity even if the organization itself is being under-served. However, by defining a hierarchy, administrators can ensure that unused capacity can be first alloted to sub-queues within the organization at the same level.
        • Sub-queues can be nested. So there can be queues within a sub-queue.
        • The last queue in a hierarchy of queues is the only queue to which jobs can be submitted. In this JIRA, we'll call it leaf level queue.

        Minimum Capacities

        • A minimum capacity of a sub-queue defines what percentage of the parent's capacity it is entitled to.
        • The scheduler will pick up a sub-queue to service if it's furthest away from meeting its minimum capacity. The current definition of being furthest away is a function of currently used capacity and minimum capacity. (used-capacity/minimum-capacity)
        • By default, a queue's capacity usage is elastic and will go beyond the minimum capacity if there is unused capacity.
          Leaf level queues that are fairly important, and their parent sub-queues recursively must be granted a high minimum percentage to ensure the scheduler chooses them first.
        • The minimum capacity of a queue can be zero. Setting to zero means that this queue will be serviced ONLY IF no other queue at the same level has anything more to run (irrespective of current usage).
        • If the minimum capacity for a queue is not specified, there are two choices:
          • Defaults to zero
          • Defaults to 100 - sum (minimum capacities for queues at the same level).
            • If more than one queue has no minimum specified, this value will be equally split among all queues.

        Maximum Capacity

        • A maximum capacity defines a limit beyond which a sub-queue cannot use the capacity of its parent queue.
        • This provides a means to limit how much excess capacity a sub-queue can use. By default, there is no limit.
        • A typical use case for using a maximum capacity limit could be to curtail certain jobs which are long running in nature from occupying more than a certain percentage of the cluster, which in the absence of pre-emption, could lead to capacity guarantees of other queues being affected.
        • Naturally, the maximum capacity of a queue can only be greater than or equal to its minimum capacity.

        Scheduling among sub-queues

        When a tasktracker comes to get a task, the scheduler uses the following algorithm to assign a task to the tracker.

        • Sort all sub-queues at the first level according to a function of used-capacity and minimum-capacity.
        • Pick up the most needy queue, and see if it has work to do.
          • If this is a leaf level queue with pending tasks, a task is picked up from the jobs in the queue as long as it is within any maximum limits for the queue.
          • If this is not a leaf level queue, apply the algorithm recursively among sub-queues under this queue.
        • If no task is found, move on to the next queue in the list.
        Show
        Hemanth Yamijala added a comment - Here is a proposal that came out after some discussions for consideration: User Scenario Consider an organization "Org1". Suppose they have a queue which is allocated a capacity of 60% of the cluster resources. Org1 has the following types of jobs: Production jobs Research jobs - belonging to three projects - say Proj1, Proj2 and Proj3 Miscellaneous types of jobs. Org1 would like to have a greater control over how the 60% capacity is used effectively amongst these types of jobs. For e.g. they'd like the research jobs and miscellaneous jobs to get some minimum amount of capacity for their work, and a significant share of the capacity to be alloted to production jobs. Production jobs are submitted somewhat rarely to the cluster. Whenever they are submitted, they must get the first chance to run, followed by research jobs and then miscellaneous jobs. However, whenever there are no production jobs, the research jobs must get this unused capacity. A Sample Configuration The following is an illustrative expression of the features/changes we plan to implement in the capacity scheduler to solve the use cases mentioned in the User Scenario. The actual specification may be different from the one described here. However, effort will be made to keep it as close to this expression as possible. grid { Org1 min=60% { priority min=90% { production min=82% proj1 min=6% max=10% proj2 min=6% proj3 min=6% } miscellaneous min=10% } Org2 min=40% } Description The features introduced in the sample configuration are described below. Sub-queues Sub-queues are queues configured within queues. They provide a mechanism for administrators to link logically related queues (say on the basis of the organization they are set up for). Using sub-queues, administrators would primarily be able to use capacity allocated to their organization more effectively than in the current model. For instance, in the current implementation which has only a single level of queues, if there is unused capacity in a queue, it is divided among remaining queues in proportion to their capacities. This has the disadvantage that logically unrelated queues could stake a claim to this unused capacity even if the organization itself is being under-served. However, by defining a hierarchy, administrators can ensure that unused capacity can be first alloted to sub-queues within the organization at the same level. Sub-queues can be nested. So there can be queues within a sub-queue. The last queue in a hierarchy of queues is the only queue to which jobs can be submitted. In this JIRA, we'll call it leaf level queue. Minimum Capacities A minimum capacity of a sub-queue defines what percentage of the parent's capacity it is entitled to. The scheduler will pick up a sub-queue to service if it's furthest away from meeting its minimum capacity. The current definition of being furthest away is a function of currently used capacity and minimum capacity. (used-capacity/minimum-capacity) By default, a queue's capacity usage is elastic and will go beyond the minimum capacity if there is unused capacity. Leaf level queues that are fairly important, and their parent sub-queues recursively must be granted a high minimum percentage to ensure the scheduler chooses them first. The minimum capacity of a queue can be zero. Setting to zero means that this queue will be serviced ONLY IF no other queue at the same level has anything more to run (irrespective of current usage). If the minimum capacity for a queue is not specified, there are two choices: Defaults to zero Defaults to 100 - sum (minimum capacities for queues at the same level). If more than one queue has no minimum specified, this value will be equally split among all queues. Maximum Capacity A maximum capacity defines a limit beyond which a sub-queue cannot use the capacity of its parent queue. This provides a means to limit how much excess capacity a sub-queue can use. By default, there is no limit. A typical use case for using a maximum capacity limit could be to curtail certain jobs which are long running in nature from occupying more than a certain percentage of the cluster, which in the absence of pre-emption, could lead to capacity guarantees of other queues being affected. Naturally, the maximum capacity of a queue can only be greater than or equal to its minimum capacity. Scheduling among sub-queues When a tasktracker comes to get a task, the scheduler uses the following algorithm to assign a task to the tracker. Sort all sub-queues at the first level according to a function of used-capacity and minimum-capacity. Pick up the most needy queue, and see if it has work to do. If this is a leaf level queue with pending tasks, a task is picked up from the jobs in the queue as long as it is within any maximum limits for the queue. If this is not a leaf level queue, apply the algorithm recursively among sub-queues under this queue. If no task is found, move on to the next queue in the list.
        Hide
        rahul k singh added a comment -

        Uploaded the patch:
        Patch implements the model discussed above. This is first cut of the implementation.

        Some Class level details:
        ======================
        The hierarchial queue follows composite pattern.

        Component
        ----------------
        AbstractQueue :
        ------------------------

        • This is abstraction for all the queues.
        • implements default behavior for some of the abstraction.
        • declares interfaces to access the children and its components.
        • defines an interface for accessing a queue's parent in the recursive structure.

        Composite
        -----------------
        ContainerQueue:

        • This provides functionality mentioned as the part of Sub-Queues.
        • consists of composition of AbstractQueue.
        • provides functionality to manipulate children
        • delegates respective children component calls.

        Leaf
        ----------
        JobQueue:

        • This provides functionality similar to Leaf Queue mentioned in the document above.
        • Jobs would be submitted only to this queue.
        • Consists of list of jobs and acts on the JobInProgressListener events.

        JobQueuesManager no more maintains list of runningJob and waitingJobs , they are maintained by JobQueue. It simply delegates
        the call to JobQueue.

        AbstractQueue have QueueSchedulingContext ,defined , it consists of all the queue related information required for scheduling decision.

        Some refactoring has been done in terms of moving out some of the inner classes from CapacityTaskScheduler , esp the QueueSchedulingInfo and TaskSchedulingInfo.

        I ll be uploading patch with testcases.

        Show
        rahul k singh added a comment - Uploaded the patch: Patch implements the model discussed above. This is first cut of the implementation. Some Class level details: ====================== The hierarchial queue follows composite pattern. Component ---------------- AbstractQueue : ------------------------ This is abstraction for all the queues. implements default behavior for some of the abstraction. declares interfaces to access the children and its components. defines an interface for accessing a queue's parent in the recursive structure. Composite ----------------- ContainerQueue: This provides functionality mentioned as the part of Sub-Queues. consists of composition of AbstractQueue. provides functionality to manipulate children delegates respective children component calls. Leaf ---------- JobQueue: This provides functionality similar to Leaf Queue mentioned in the document above. Jobs would be submitted only to this queue. Consists of list of jobs and acts on the JobInProgressListener events. JobQueuesManager no more maintains list of runningJob and waitingJobs , they are maintained by JobQueue. It simply delegates the call to JobQueue. AbstractQueue have QueueSchedulingContext ,defined , it consists of all the queue related information required for scheduling decision. Some refactoring has been done in terms of moving out some of the inner classes from CapacityTaskScheduler , esp the QueueSchedulingInfo and TaskSchedulingInfo. I ll be uploading patch with testcases.
        Hide
        Sreekanth Ramakrishnan added a comment -

        Started taking look at the patch, following are comments:

        QueueSchedulingContext:

        • Can a comment be added for the each of the fields of the QueueSchedulingContext, stating what each is meant to do?
          AbstractQueue
        • Change cummulative to cumulative.
        • Childrens should be children
        • Can we move the queue scheduling information string into QueueSchedulingContext? As we have for the TaskSchedulingContext?
        • Can we take away state variables prevReduceClusterCapacity and prevMapClusterCapacity so there is no state maintained?

        JobQueue:-

        • Why are we having the jobStateChanged method in JobQueue, shouldnt it be in JobQueueManager which is JobInProgressListener and shouldn't it be handling the job state change event and handling and management of movement of jobs?

        JobInitalizationPoller:
        *Do we need to change the log statements? (Queue to AbstractQueue? Or Should it be JobQueue?)

        • What would QueueManager.getQueues() give to JobInitializationPoller in CapacityScheduler.start()
        Show
        Sreekanth Ramakrishnan added a comment - Started taking look at the patch, following are comments: QueueSchedulingContext: Can a comment be added for the each of the fields of the QueueSchedulingContext, stating what each is meant to do? AbstractQueue Change cummulative to cumulative. Childrens should be children Can we move the queue scheduling information string into QueueSchedulingContext? As we have for the TaskSchedulingContext? Can we take away state variables prevReduceClusterCapacity and prevMapClusterCapacity so there is no state maintained? JobQueue:- Why are we having the jobStateChanged method in JobQueue, shouldnt it be in JobQueueManager which is JobInProgressListener and shouldn't it be handling the job state change event and handling and management of movement of jobs? JobInitalizationPoller: *Do we need to change the log statements? (Queue to AbstractQueue? Or Should it be JobQueue?) What would QueueManager.getQueues() give to JobInitializationPoller in CapacityScheduler.start()
        Hide
        Sreekanth Ramakrishnan added a comment -

        To clarify the above comment was:

        What would QueueManager.getQueues() give to JobInitializationPoller in CapacityScheduler.start()

        The JobInitalizationPoller acts only on JobQueue not ContainerQueue which is configured by the QueueManager.

        I agree, the future patch would address configuration related changes, but atleast interim the patch should see to that JobInitalizationPoller would get only JobQueue.

        Show
        Sreekanth Ramakrishnan added a comment - To clarify the above comment was: What would QueueManager.getQueues() give to JobInitializationPoller in CapacityScheduler.start() The JobInitalizationPoller acts only on JobQueue not ContainerQueue which is configured by the QueueManager . I agree, the future patch would address configuration related changes, but atleast interim the patch should see to that JobInitalizationPoller would get only JobQueue .
        Hide
        rahul k singh added a comment -

        This patch consists of the testcases for hierarchial queues

        Addressed the issues raised by sreekanth.

        Show
        rahul k singh added a comment - This patch consists of the testcases for hierarchial queues Addressed the issues raised by sreekanth.
        Hide
        rahul k singh added a comment -

        previous patch missed out the newly added testcases.
        adding them

        Show
        rahul k singh added a comment - previous patch missed out the newly added testcases. adding them
        Hide
        Hemanth Yamijala added a comment -

        Some comments:

        • AbstractQueue.updateContext can move into QueueSchedulingContext, as all the state it is operating on is in QSC.
        • Also prevMapClusterCapacity and prevReduceClusterCapacity can also be moved to the context. They can be private, and renamed to prev*Capacity, dropping the 'Cluster' because for container queues, they don't reflect the entire cluster capacity. Same naming change would apply to variables in QueueSchedulingContext (like setMapClusterCapacity, etc)
        • AbstractQueue.getOrderedJobQueues, its not very clear that this is looking through the entire hierarchy. Also, it assumes that sorting is done before this. So, its not a very orthogonal API. Move this to the scheduler, and introduce a new API like AbstractQueue.getDescendentJobQueues().
        • Override AbstractQueue.addChildren in JobQueue to throw an unsupported exception.
        • Make AbstractQueue.getChildren package private and document it is for tests.
        • I suggest we modify the algorithm in distributeUnConfiguredCapacity to follow this pattern to make it clearer:
          for (Queue q : children) {
            if (q.capacity == -1) {
              unconfigured.add(q);
            }
          }
          
          // distribute capacity for all unconfigured queues.
          
          for (Queue q : children) {
            q.distributeUnconfiguredCapacity();
          }
          
        • I would suggest we provide equals and hashCode in AbstractQueue to be based on the queue Name. toString in AbstractQueue should print the queue name.
        • I didn't understand the need for setting the capacity in conf in distributeUnconfiguredCapacity. It seems like requiring the Configuration instance to be passed to distributeUnconfiguredCapacity is creating an undesirable dependency. Can you check if we can break this dependency.
        • distributeUnConfiguredCapacity will throw a Divide by zero if there is no queues without configured capacity.
        • We don't need to pass the supportsPriority variable separately to the JobQueue's constructor. Let's set that directly in the JobQueue.QueueSchedulingContext which we are already passing to JobQueue.
        • In JobQueue, methods like addWaitingJob etc should be private. Also, I think some of the methods can be folded. For e.g. makeJobRunning just calls addRunningJob, so we can refactor to remove makeJobRunning and call addRunningJob directly.
        • TaskData seems out of place in TaskSchedulingContext. The scheduling context contains state w.r.to scheduler. TaskData is a simple abstraction that returns a view of job information based on the task type. So, let's pull it out and call it TaskDataView which can be extended by MapTaskDataView and ReduceTaskDataView. There should be only one ‌instance of these per scheduler instance and they can be got from the scheduler itself.
        • Rename TaskSchedulingContext.add to TSC.update.
        • Can we pull out the whole hierarchy building logic into a separate class - like a QueueHierarchyBuilder ? It could be given the CapacitySchedulerConf and QueueManager and have an API like buildHierarchy - which would return the root of the queues. Capacity scheduler can thus be abstracted from how the hierarchy is created - it just gets the hierarchy from somewhere. For e.g. in tests, the hierarchy can be manually created and given to the given.
        • Please remove mapScheduler.initialize() and reduceScheduler.initialize().
        • tsi.getMaxCapacity() < tsi.getCapacity(): this check in areTasksInQueueOverLimit does not seem required. Because the check is already being done in tsi.getCapacity()
        • totalCapacity modification in the loadContext is a no-op, because the changes will not be reflected in the caller method. Likewise the check for totalCapacity > 100.0 is a no-op in createHierarchy.
        • The separator char for queues is chosen to be '.' in createHierarchy. It must be checked that this character doesn't appear anywhere else in the queue name.
        • Call to root.sort() should be from TaskSchedulingMgr.assignTasks()
        • JobQueuesManager.createQueue should be addQueue. Also, it can get the queue name from the job queue object directly, and doesn't need the extra parameter.
        • JobQueueManager.getQueueNames can be getJobQueueNames.
        Show
        Hemanth Yamijala added a comment - Some comments: AbstractQueue.updateContext can move into QueueSchedulingContext, as all the state it is operating on is in QSC. Also prevMapClusterCapacity and prevReduceClusterCapacity can also be moved to the context. They can be private, and renamed to prev*Capacity, dropping the 'Cluster' because for container queues, they don't reflect the entire cluster capacity. Same naming change would apply to variables in QueueSchedulingContext (like setMapClusterCapacity, etc) AbstractQueue.getOrderedJobQueues, its not very clear that this is looking through the entire hierarchy. Also, it assumes that sorting is done before this. So, its not a very orthogonal API. Move this to the scheduler, and introduce a new API like AbstractQueue.getDescendentJobQueues(). Override AbstractQueue.addChildren in JobQueue to throw an unsupported exception. Make AbstractQueue.getChildren package private and document it is for tests. I suggest we modify the algorithm in distributeUnConfiguredCapacity to follow this pattern to make it clearer: for (Queue q : children) { if (q.capacity == -1) { unconfigured.add(q); } } // distribute capacity for all unconfigured queues. for (Queue q : children) { q.distributeUnconfiguredCapacity(); } I would suggest we provide equals and hashCode in AbstractQueue to be based on the queue Name. toString in AbstractQueue should print the queue name. I didn't understand the need for setting the capacity in conf in distributeUnconfiguredCapacity. It seems like requiring the Configuration instance to be passed to distributeUnconfiguredCapacity is creating an undesirable dependency. Can you check if we can break this dependency. distributeUnConfiguredCapacity will throw a Divide by zero if there is no queues without configured capacity. We don't need to pass the supportsPriority variable separately to the JobQueue's constructor. Let's set that directly in the JobQueue.QueueSchedulingContext which we are already passing to JobQueue. In JobQueue, methods like addWaitingJob etc should be private. Also, I think some of the methods can be folded. For e.g. makeJobRunning just calls addRunningJob, so we can refactor to remove makeJobRunning and call addRunningJob directly. TaskData seems out of place in TaskSchedulingContext. The scheduling context contains state w.r.to scheduler. TaskData is a simple abstraction that returns a view of job information based on the task type. So, let's pull it out and call it TaskDataView which can be extended by MapTaskDataView and ReduceTaskDataView. There should be only one ‌instance of these per scheduler instance and they can be got from the scheduler itself. Rename TaskSchedulingContext.add to TSC.update. Can we pull out the whole hierarchy building logic into a separate class - like a QueueHierarchyBuilder ? It could be given the CapacitySchedulerConf and QueueManager and have an API like buildHierarchy - which would return the root of the queues. Capacity scheduler can thus be abstracted from how the hierarchy is created - it just gets the hierarchy from somewhere. For e.g. in tests, the hierarchy can be manually created and given to the given. Please remove mapScheduler.initialize() and reduceScheduler.initialize(). tsi.getMaxCapacity() < tsi.getCapacity(): this check in areTasksInQueueOverLimit does not seem required. Because the check is already being done in tsi.getCapacity() totalCapacity modification in the loadContext is a no-op, because the changes will not be reflected in the caller method. Likewise the check for totalCapacity > 100.0 is a no-op in createHierarchy. The separator char for queues is chosen to be '.' in createHierarchy. It must be checked that this character doesn't appear anywhere else in the queue name. Call to root.sort() should be from TaskSchedulingMgr.assignTasks() JobQueuesManager.createQueue should be addQueue. Also, it can get the queue name from the job queue object directly, and doesn't need the extra parameter. JobQueueManager.getQueueNames can be getJobQueueNames.
        Hide
        Hemanth Yamijala added a comment -

        Looked at the test cases:

        • Code seems duplicated between CapacitySchedulerUtils and CapacityTaskScheduler and TestContainerQueue.
        • In some test cases, when we create a queue, it is already adding a child to the parent. So, why do we need additional calls to addChildren ?
        • What's the difference between testConfiguredCapacity and testMinCapacity ?
        • The test cases testing scheduling are nice. The comments are out of sync a bit, and will be hard to maintain. Instead I suggest that we assert what we are documenting in the tests itself, so that they themselves read as comments, and will also always be in sync.
        • As discussed, getCapacity() should not return max capacity any time. It should always return the current capacity or limit, whichever is smaller. Otherwise, the sort order of queues would be affected.
        • areTasksInQueueOverLimit should be changed to something along these lines:
                if (tsi.getMaxTaskLimit() > 0) {
                  if (tsi.getNumSlotsOccupied() >= tsi.getCapacity()) {
                    return true;
                  }
                } 
                
                if (tsi.getMaxCapacity() > 0) {
                  if (tsi.getNumSlotsOccupied() >= tsi.getMaxCapacity()) {
                    return true;
                  }
                }
                return false;
          
        • At the same time, testMaxCapacity should be removed. I would instead recommend a test case that sets a max capacity on a queue, and checks scheduling honors the decision.
        Show
        Hemanth Yamijala added a comment - Looked at the test cases: Code seems duplicated between CapacitySchedulerUtils and CapacityTaskScheduler and TestContainerQueue. In some test cases, when we create a queue, it is already adding a child to the parent. So, why do we need additional calls to addChildren ? What's the difference between testConfiguredCapacity and testMinCapacity ? The test cases testing scheduling are nice. The comments are out of sync a bit, and will be hard to maintain. Instead I suggest that we assert what we are documenting in the tests itself, so that they themselves read as comments, and will also always be in sync. As discussed, getCapacity() should not return max capacity any time. It should always return the current capacity or limit, whichever is smaller. Otherwise, the sort order of queues would be affected. areTasksInQueueOverLimit should be changed to something along these lines: if (tsi.getMaxTaskLimit() > 0) { if (tsi.getNumSlotsOccupied() >= tsi.getCapacity()) { return true ; } } if (tsi.getMaxCapacity() > 0) { if (tsi.getNumSlotsOccupied() >= tsi.getMaxCapacity()) { return true ; } } return false ; At the same time, testMaxCapacity should be removed. I would instead recommend a test case that sets a max capacity on a queue, and checks scheduling honors the decision.
        Hide
        rahul k singh added a comment -

        Implemented all the changes suggested above
        The patch consist of everything except the new recommended testMaxCapacity

        Show
        rahul k singh added a comment - Implemented all the changes suggested above The patch consist of everything except the new recommended testMaxCapacity
        Hide
        rahul k singh added a comment -

        The comment regarding algorithm for distribution of unconfigured capacity , is not implemented as , current implementation is more efficient.
        Will be adding 1 more patch with test for max capacity.

        Show
        rahul k singh added a comment - The comment regarding algorithm for distribution of unconfigured capacity , is not implemented as , current implementation is more efficient. Will be adding 1 more patch with test for max capacity.
        Hide
        rahul k singh added a comment -

        Attaching the new file with testMaxCapacity testcase

        Show
        rahul k singh added a comment - Attaching the new file with testMaxCapacity testcase
        Hide
        Hemanth Yamijala added a comment -

        This is getting better. I do have some more feedback:

        • updateStatsOnRunningJob, addRunningJob, removeRunningJob, removeWaitingJob - make private
        • ASF licence header should be the first in the src file.
        • Replace sortJobQueues with inline method.
        • QueueHierarchyBuilder is creating a new instance of the CapacityTaskScheduler, which is unnecessary.
        • static builder instance also seems unnecessary.
        • In QueueHierarchyBuilder, when checking for separator char, IllegalArgumentException must show the queue name which failed the check.
        • Discuss: Back dependency between QueueHierarchyBuilder and Scheduler - can this be avoided.
        • AbstractQueue does not override equals, while hashcode is overridden. Also, the toString API was previously printing other information. I'd only asked the name of the queue to be prepended to it, not to remove the other information.
        • It is a little confusing that the number of slots being asserted after task assignment does not include the currently scheduled task. Recommend to move the asserts before assignment.
        • Root should always be set up only in a certain way. I would recommend, there's a single static instance of root, which is always got from the capacity scheduler, even in tests.
        • In testMaxCapacity, rt.update in tests should send in the capacity of the clusters to be in sync.
        • getTaskDataView() need not be in TaskSchedulingContext. Since it is static, it can be called directly from other classes like the scheduler, passing the type.
        • AbstractQueue.addChildren should be addChild.

        Some of the earlier comments are not taken:

        • APIs in JobQueuesManager and JobQueue can be folded still.
        • mapTSI and reduceTSI member variables of JobQueue are not needed.
        • AbstractQueue.getChildren is still public
        • getCapacity() should not return max capacity any time. It should always return the current capacity or limit, whichever is smaller.
        Show
        Hemanth Yamijala added a comment - This is getting better. I do have some more feedback: updateStatsOnRunningJob, addRunningJob, removeRunningJob, removeWaitingJob - make private ASF licence header should be the first in the src file. Replace sortJobQueues with inline method. QueueHierarchyBuilder is creating a new instance of the CapacityTaskScheduler, which is unnecessary. static builder instance also seems unnecessary. In QueueHierarchyBuilder, when checking for separator char, IllegalArgumentException must show the queue name which failed the check. Discuss: Back dependency between QueueHierarchyBuilder and Scheduler - can this be avoided. AbstractQueue does not override equals, while hashcode is overridden. Also, the toString API was previously printing other information. I'd only asked the name of the queue to be prepended to it, not to remove the other information. It is a little confusing that the number of slots being asserted after task assignment does not include the currently scheduled task. Recommend to move the asserts before assignment. Root should always be set up only in a certain way. I would recommend, there's a single static instance of root, which is always got from the capacity scheduler, even in tests. In testMaxCapacity, rt.update in tests should send in the capacity of the clusters to be in sync. getTaskDataView() need not be in TaskSchedulingContext. Since it is static, it can be called directly from other classes like the scheduler, passing the type. AbstractQueue.addChildren should be addChild. Some of the earlier comments are not taken: APIs in JobQueuesManager and JobQueue can be folded still. mapTSI and reduceTSI member variables of JobQueue are not needed. AbstractQueue.getChildren is still public getCapacity() should not return max capacity any time. It should always return the current capacity or limit, whichever is smaller.
        Hide
        Hemanth Yamijala added a comment -

        Attached patch incorporates all comments I raised, except the following:

        • JobQueue.removeWaitingJob is used by JobInitializationPoller. Hence, retained the access level as package private.
        • I also did not fold methods in JobQueue and JobQueuesManager since most seemed to be used at more than one place.

        In addition to the comments, Rahul and I also found and fixed a bug in getMaxCapacity() and changed the default value of the same to -1 in the capacity-scheduler.xml.template.

        Rahul, can you please take a look at the changes and make sure you are fine with them ?

        Show
        Hemanth Yamijala added a comment - Attached patch incorporates all comments I raised, except the following: JobQueue.removeWaitingJob is used by JobInitializationPoller. Hence, retained the access level as package private. I also did not fold methods in JobQueue and JobQueuesManager since most seemed to be used at more than one place. In addition to the comments, Rahul and I also found and fixed a bug in getMaxCapacity() and changed the default value of the same to -1 in the capacity-scheduler.xml.template. Rahul, can you please take a look at the changes and make sure you are fine with them ?
        Hide
        Hemanth Yamijala added a comment -

        Trying hudson.

        Show
        Hemanth Yamijala added a comment - Trying hudson.
        Hide
        rahul k singh added a comment -

        Some minor nitpickings:

        In QueueHierarchyBuilder variable scheduler is never used.
        In QueueHierarchyBuilder loadContext has parameter of QueueManager which is not used .changing the signature of loadContext to
        only take queue name. same with createHierarchy method.

        Some of the testcases had some System.out.println which are debug statements removing those.

        Show
        rahul k singh added a comment - Some minor nitpickings: In QueueHierarchyBuilder variable scheduler is never used. In QueueHierarchyBuilder loadContext has parameter of QueueManager which is not used .changing the signature of loadContext to only take queue name. same with createHierarchy method. Some of the testcases had some System.out.println which are debug statements removing those.
        Hide
        rahul k singh added a comment -

        Uploaded the new patch with comments mentioned above .

        Show
        rahul k singh added a comment - Uploaded the new patch with comments mentioned above .
        Hide
        Hemanth Yamijala added a comment -

        +1 for the changes. Thanks for catching the changes in QueueHierarchyBuilder.

        Show
        Hemanth Yamijala added a comment - +1 for the changes. Thanks for catching the changes in QueueHierarchyBuilder.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12417611/MAPREDUCE-824-6.patch
        against trunk revision 807640.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 16 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/518/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/518/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/518/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/518/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12417611/MAPREDUCE-824-6.patch against trunk revision 807640. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 16 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/518/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/518/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/518/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/518/console This message is automatically generated.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12417701/MAPREDUCE-824-7.patch
        against trunk revision 807954.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 16 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/520/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/520/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/520/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/520/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12417701/MAPREDUCE-824-7.patch against trunk revision 807954. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 16 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/520/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/520/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/520/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/520/console This message is automatically generated.
        Hide
        Hemanth Yamijala added a comment -

        I just committed this to trunk. Thanks, Rahul !

        Show
        Hemanth Yamijala added a comment - I just committed this to trunk. Thanks, Rahul !
        Hide
        Arun C Murthy added a comment -

        Editorial pass over 0.21/0.22/0.23 content for hadoop-0.23.

        Show
        Arun C Murthy added a comment - Editorial pass over 0.21/0.22/0.23 content for hadoop-0.23.

          People

          • Assignee:
            rahul k singh
            Reporter:
            Hemanth Yamijala
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development