[jira] Commented: (HADOOP-4665) Add preemption to the fair scheduler

["Vinod K V (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HADOOP-4665?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12694524#action_12694524 ] 

Vinod K V commented on HADOOP-4665:
-----------------------------------

Here's a more comprehensive review:

Major points:
 - As it's a new feature, I think preemption(mapred.fairscheduler.preemption) should be disabled(false) by default.
 - Can we do something like above stated proposal: to preempt a task when a job's fairshare/minshare are not met within PREEMPTION_TIMEOUT leaving 2/3 heartbeats for the task to actually get killed.
 - DUMP_INTERVAL and PREEMPTION_INTERVAL should be configurable. The variables themselves can be package-private instead of public.
 - The class FairSchedulerEventLog can just be package-private. So do all the methods inside - init, log, shutdown and isEnabled - they don't need to be public as of now.

 - FairScheduler.preemptTasksIfNecessary() method:
   -- This method does a Collections.reverse(jobs) after sorting the jobs. We can just traverse the list in reverse order to get the desired effect here.
   --  The check as to whether FairScheduler will do preemption(preemptionEnabled && !useFifo) is done deep inside - all the stats are calculated and then only preemption is skipped if not needed. Can we take this check, may be, to the beginning of preemptTasksIfNecessary() method or inside update() method itself.

 - FairScheduler.tasksToPreempt() method:
   -- The count tasksDueToFairShare seems to be calculated to see if full fair-share of slots are allotted or not instead of the advertised half of fairshare. I think this is a mistake as isStarvedForFairShare() is checking for half of fair-share. Or am I missing something?
   -- EventLogs in this method are a bit confusing if the job is short on both minshare and fairshare. In this case, it is roughly giving an impression that we are preempting twice. I think it would be clear to just log that the job is short by so many slots w.r.t minshare and w.r.t fairshare. And in the end, just before returning we can simply say the exact number of slots we are going to preempt.

 - FairScheduler.start() method:
   -- I think that the call loadMgr.setEventLog(eventLog) should be after eventLog is initialized with a new FairSchedulerEvenLog object.

 - TestFairScheduler.java:
  -- You have added setup and cleanup tasks creation code in initTasks() method. Is there any specific reason for doing this? In any case, much of it duplicates code from JobInProgress.initTasks(), if we really want, we can refactor this code into a new method say JobInProgress.createSpecialTasks().
  -- There are no new test-cases related to preemption. I think we should have one/some.

Minor nits:
 - Javadoc for getFairSharePreemptionTimeout() is incomplete. Also, if I am understanding correctly FairSharePreemptionTimeout is the same for all pools/jobs.
 - The xml tag for minSharePreemptionTimeouts is currently preemptionTimeout. It can better be minSharePreemptionTimeout.
 - update thread:  The log message "Failed to update fair share calculations" can better be "Exception in Update thread". This because,now , update thread does more than just updating calculations.

> Add preemption to the fair scheduler
> ------------------------------------
>
>                 Key: HADOOP-4665
>                 URL: https://issues.apache.org/jira/browse/HADOOP-4665
>             Project: Hadoop Core
>          Issue Type: New Feature
>          Components: contrib/fair-share
>            Reporter: Matei Zaharia
>            Assignee: Matei Zaharia
>             Fix For: 0.21.0
>
>         Attachments: fs-preemption-v0.patch, hadoop-4665-v1.patch, hadoop-4665-v1b.patch, hadoop-4665-v2.patch, hadoop-4665-v3.patch, hadoop-4665-v4.patch
>
>
> Task preemption is necessary in a multi-user Hadoop cluster for two reasons: users might submit long-running tasks by mistake (e.g. an infinite loop in a map program), or tasks may be long due to having to process large amounts of data. The Fair Scheduler (HADOOP-3746) has a concept of guaranteed capacity for certain queues, as well as a goal of providing good performance for interactive jobs on average through fair sharing. Therefore, it will support preempting under two conditions:
> 1) A job isn't getting its _guaranteed_ share of the cluster for at least T1 seconds.
> 2) A job is getting significantly less than its _fair_ share for T2 seconds (e.g. less than half its share).
> T1 will be chosen smaller than T2 (and will be configurable per queue) to meet guarantees quickly. T2 is meant as a last resort in case non-critical jobs in queues with no guaranteed capacity are being starved.
> When deciding which tasks to kill to make room for the job, we will use the following heuristics:
> - Look for tasks to kill only in jobs that have more than their fair share, ordering these by deficit (most overscheduled jobs first).
> - For maps: kill tasks that have run for the least amount of time (limiting wasted time).
> - For reduces: similar to maps, but give extra preference for reduces in the copy phase where there is not much map output per task (at Facebook, we have observed this to be the main time we need preemption - when a job has a long map phase and its reducers are mostly sitting idle and filling up slots).

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.