[
https://issues.apache.org/jira/browse/MAPREDUCE-706?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12737425#action_12737425
]
Aaron Kimball commented on MAPREDUCE-706:
-----------------------------------------
Matei,
Great documentation -- that really helps! :) Also good that you added a lot of
tests. +1 overall on this patch, subject to the following (relatively minor)
questions and suggestions:
TestFairScheduler.obtainNewReduceTask():
Task task = new ReduceTask("", attemptId, 0, maps.length, 1) <-- shouldn't this
be "reduces.length" ?
TestFairScheduler.getLocalityLevel(): These locality level constants are used
throughout the FairScheduler; they should be converted to an Enum. (Magic
constants are evil.)
TestComputeFairShares.testEmptyList() -- should this call verifyShares() after
computeFairShares() to assert that the list length is zero?
PoolManager.parseSchedulingMode(): why case sensitive 'fifo' and 'fair' ? maybe
use toLower() ?
PoolSchedulable c'tor: scheduler.getClock().getTime() should be called only
once to guarantee this.lastTimeAtMinShare == this.lastTimeAtHalfFairShare on
start?
assignTask(): Is SchedulingMode guaranteed to never be extended by another
internal algorithm? If not, turn "else" into "else if" and have an "else throw
InvalidArgumentException" at the end of the case.
JobSchedulable.updateDemand(): why does this use System.currentTimeMillis()
instead of getting the time from a Clock object?
Schedulable's class javadoc: typo "algoirthms"
SchuldingAlgorithms.LOG: rather than use a string, use
SchedulingAlgorithms.class.getName()
FairScheduler.UpdateThread.run(): why is preemptTasksIfNecessary() commented
out? Needs a comment for rationale.
FairScheduler.assignTasks() -- Should convert System.out.println to log msg.
This method is also getting pretty long. Consider refactoring the inner loop
into shorter methods if you need to add anything else to it in the future.
getAllowedLocalityLevel():
You have the comment: // Job not in infos (shouldn't happen)-
... So throw an exception if it does, or at least log this event with level
ERROR, rather than returning an in-bounds value? When you get to
switch(info.lastMapLocalityLevel), you'll naturally throw an NPE, so the caller
should just deal with that and clean up its own mess.
> Support for FIFO pools in the fair scheduler
> --------------------------------------------
>
> Key: MAPREDUCE-706
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-706
> Project: Hadoop Map/Reduce
> Issue Type: New Feature
> Components: contrib/fair-share
> Reporter: Matei Zaharia
> Assignee: Matei Zaharia
> Attachments: fsdesigndoc.pdf, fsdesigndoc.tex, mapreduce-706.patch,
> mapreduce-706.v1.patch, mapreduce-706.v2.patch
>
>
> The fair scheduler should support making the internal scheduling algorithm
> for some pools be FIFO instead of fair sharing in order to work better for
> batch workloads. FIFO pools will behave exactly like the current default
> scheduler, sorting jobs by priority and then submission time. Pools will have
> their scheduling algorithm set through the pools config file, and it will be
> changeable at runtime.
> To support this feature, I'm also changing the internal logic of the fair
> scheduler to no longer use deficits. Instead, for fair sharing, we will
> assign tasks to the job farthest below its share as a ratio of its share.
> This is easier to combine with other scheduling algorithms and leads to a
> more stable sharing situation, avoiding unfairness issues brought up in
> MAPREDUCE-543 and MAPREDUCE-544 that happen when some jobs have long tasks.
> The new preemption (MAPREDUCE-551) will ensure that critical jobs can gain
> their fair share within a bounded amount of time.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
