hadoop-common-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vivek Ratan (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HADOOP-4981) Prior code fix in Capacity Scheduler prevents speculative execution in jobs
Date Tue, 13 Jan 2009 09:40:59 GMT

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

Vivek Ratan commented on HADOOP-4981:
-------------------------------------

For the sake of this discussion, let's consider my original proposal (which modifies JobInProgress.findNewXXXTask(),
and for which a patch is provided) as Proposal 1, and your suggestion about detecting only
if a job has speculative tasks as Proposal 2. 

I did consider Proposal 2, but I frankly didn't see a big difference. If you look at the code
for JobInProgress.findNewMapTask(), which really is the big core method that needs to be updated,
the first part of it deals with finding a pending task to run, and the second part of it deals
with finding a speculative task. The latter code is non-trivial, i.e., it involves 3 stages
and about 60 lines of code. Proposal 2 will result in fewer if-blocks, but the changes will
qualitatively be the same (i.e., you'll still have to pass the readOnly parameter and still
wrap data structure changes and log methods with if-blocks, albeit only for these 60 lines).
A quick glance at my patch shows that you'll cut down the if-blocks from about 17 to about
7. On one hand, that is a decrease, but it's not qualitatively different and you'll likely
move the code to find speculative tasks into a separate method, thus adding to the refactoring
of this core method. Either way, it looks like we'll have to bite the bullet no matter what,
if you're considering only these two proposals. 

One issue I do have with Proposal 2 is that we're replicating the logic of finding a task
to run, in the Capacity Scheduler. In your code example, you're assuming that if a job does
not have pending or speculative tasks, JobInProgress.obtainNewMapTask() will return null.
That's not such a good assumption for a couple of related reasons: 
a. if this logic changes in JobInProgress, you have to update the Capacity Scheduler code.
Granted that this logic is likely not going to change, or that eventually all this code belongs
in a Scheduler module anyways, so maybe this is not a big issue. 
b. findNewMapTask() has additional logic that you should consider: whether the TT is blacklisted
(shouldRunOnTaskTracker()), and whether the TT has enough disk space. The latter we probably
don't care about much, because the disk space, like the amount of free memory, gets better
if we let the TT finish its tasks. The former,however, is important. If no task in the job
can run on the TT (because many tasks of the job have failed on that TT), then the scheduler
logic needs to reflect that (i.e., the scheduler blocks if there are pending or speculative
tasks AND the TT is not blacklisted). Which ties in to my first point: you're replicating
the logic of findNewMapTask() in the scheduler, which doesn't buy you much, and if you do
it, you'll need to separate out the various steps in the logic (finding a pending tasks, finding
a speculative task ) into separate methods, and make JobInProgress.shouldRunOnTaskTracker()
public.

Basically, I don't see Proposal 2 as qualitatively better than Proposal 1, which is why I
didn't consider it. In its favor, it cuts down the if-blocks by more than half. But is that
enough reason to replicate the findNewMapTask() logic in the scheduler? I personally think
not. 

Also remember that in proposal 1, we're not altering the signature of JobInProgress in the
sense that its current callers do not change. It still exposes obtainNewMapTask(), with the
same parameters. We've added a new method, hasNewMapTask(). It's only internally that we use
the ugly readOnly flag. A minor point, but I wanted to point it out.

bq. This may require an API like findSpeculativeTask which only looks at code dealing with
speculative tasks, and does not need to make changes to the core APIs like obtainNewMapTask

Depends on how you see it. The code that deals with finding speculative tasks is, today, within
the core API findNewMapTask(). You'll still affect the core API if you move it out, but yes,
I get your point that you won't have to surround the remaining code by if-blocks. 


> Prior code fix in Capacity Scheduler prevents speculative execution in jobs
> ---------------------------------------------------------------------------
>
>                 Key: HADOOP-4981
>                 URL: https://issues.apache.org/jira/browse/HADOOP-4981
>             Project: Hadoop Core
>          Issue Type: Bug
>          Components: contrib/capacity-sched
>            Reporter: Vivek Ratan
>            Priority: Blocker
>         Attachments: 4981.1.patch
>
>
> As part of the code fix for HADOOP-4035, the Capacity Scheduler obtains a task from JobInProgress
(calling obtainNewMapTask() or obtainNewReduceTask()) only if the number of pending tasks
for a job is greater than zero (see the if-block in TaskSchedulingMgr.getTaskFromJob()). So,
if a job has no pending tasks and only has running tasks, it will never be given a slot, and
will never have a chance to run a speculative task. 

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


Mime
View raw message