|
I think that the tasks should include a boolean in the done message to the task tracker that says if they have output to promote. (And it should delete everything in the case of failure, locally.) This is just an optimization. The framework (TaskTracker.Child.main) would look in the work output directory and set true if there is anything to promote. The TT would then set the state to commit-pending or success according to the flag value.
Owen O'Malley made changes - 31/Mar/08 09:42 PM
How about this
1) Task.done() method checks if the task has data to be promoted and passes this info to the TaskTracker via the TaskTracker.done() api. 2) If there is no data to promote, the TaskTracker sets the task status as SUCCEEDED or FAILED depending on whether the task succeeds or fails. 3) JobInProgress adds only COMMIT_PENDING tasks to the commit-pending queue. The commit-pending queue deals with KILLED/FAILED tasks only if the commit-pending thread fails to save the task output or if the TaskTracker is lost. 4) Temporary data from FAILED/KILLED tasks will be deleted once the job completes (see 5) JobInProgress.updateTaskStatus() can now be called with SUCCEEDED state from TaskTracker (via heartbeat) or from the commit-pending queue. 5) If a JobInProgress.updateTaskStatus() is called with SUCCEEDED state for a completed TIP it will be marked as KILLED.
+1 In addition, we should discard outputs of failed tasks in TaskTracker.Child.main if feasible in the 'finally' clause in TaskTracker.Child.main. Then we could just set the status to 'FAILED/KILLED' and relieve of the need to discard outputs in a lot of cases. We could go further and do the same in the TT too to ensure that the JT only needs to promote outputs of successful tasks... clearly it needs some careful thought.
Amar Kamat made changes - 02/Apr/08 06:17 AM
Amar Kamat made changes - 02/Apr/08 06:17 AM
We actually don't need to discard output (at the cost of creating some temp garbage on the dfs). The jobtracker deletes the temp dir for the job at the end of the job (
Reiterating #4 from my earlier comment. Here we might ignore the failed/killed tasks and never call discard. It will be taken care once the job completes. This is a simple approach. Another approach is to have a scavenger thread that will periodically do this cleanup business offline. But for now leaving the garbage as it is and reclaiming it once the job finishes seems to be a simple/better solution.
Amar Kamat made changes - 02/Apr/08 02:00 PM
Amar Kamat made changes - 02/Apr/08 02:03 PM
Looks good, couple of comments:
1. I'm a little bothered by + // If the TIP is already completed and the task reports as SUCCEEDED then
+ // mark the task as KILLED.
+ // In case of task with no promotion the task tracker will mark the task
+ // as SUCCEEDED.
+ if (wasComplete && (status.getRunState() == TaskStatus.State.SUCCEEDED)) {
+ status.setRunState(TaskStatus.State.KILLED);
+ }
boolean change = tip.updateStatus(status);
if (change) {
TaskStatus.State state = status.getRunState();
Normally I'd expect the first check inside the 'if (change)' to make sure the same status isn't being processed twice, and wrongly manipulates the state of the TIP - I'm happy if you can confirm that this works... just being careful. 2. Please bump up TaskUmbilicalProtocol's version number. Arun, Two things
1) If the status is replayed by the TaskTracker, the JobTracker will take care of that. The JobTracker.heartbeat() will simply discard it there and then. 2) If at all the status gets replayed (in JobInProgress.updateTaskStatus()) it will be taken care as follows a) task t comes in as SUCCEEDED for a tip that is already completed. b) It will be marked (locally) as KILLED and the tasks status will be updated in the JT. c) If at all the status is resent, it will be marked locally as KILLED. Now the change in the status will result in as false and nothing will happen. The reason for marking the task as KILLED (locally) is to make sure that the semantics of the trunk is retained. If the state is updated first and later marked as KILLED then the task status will be temporarily marked as SUCCEEDED. Looks like we can optimize it further. For checking whether the task output dir is empty or not we can do the following
if (taskOutputPath != null) { // Get the file-system for the task output directory FileSystem fs = taskOutputPath.getFileSystem(conf); // Check if it exists if (fs.exists(taskOutputPath)) { // Get the summary for the folder ContentSummary summary = fs.getContentSummary(taskOutputPath); // Check if the directory contains some data // i.e total-files + total-folders - 1(itself) if ((summary.getFileCount() + summary.getDirectoryCount() - 1) > 0) { shouldBePromoted = true; } } } I have tested fs.getContentSummary() via the DFSClient and it works as expected. Comments?
Amar Kamat made changes - 03/Apr/08 02:49 PM
Amar Kamat made changes - 03/Apr/08 02:50 PM
Like Amar mentioned, it would be nice if we can eliminate the call to fs.exists() in the previous code snippet, especially if this code snippet is executed frequently. fs.getContentSummary() probably throws an exception if the file does not exists.
Dhruba, is that a documented exception. I didn't see it in the FileSystem.getContentSummary API doc. So if it is not documented is it advisable to bank client code on the exception? For e.g., what if getContentSummary, later on, returns null for non existent paths? So, unless FileSystem provides a guarantee that an exception will be thrown for non-existent paths, i'd like to go in the lines of what Amar mentioned in the code snippet. Thoughts?
> So, unless FileSystem provides a guarantee that an exception will be thrown for non-existent paths, i'd like to go in the lines of what Amar mentioned in the code snippet. Thoughts?
Then, should the code handle summary being null? (exists() is previous line does not mean it exists during next line). -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12379270/HADOOP-3140-v2.patch  against trunk revision 643282. @author +1. The patch does not contain any @author tags. tests included -1. The patch doesn't appear to include any new or modified tests. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new javac compiler warnings. release audit +1. The applied patch does not generate any new release audit warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests +1. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2148/testReport/ This message is automatically generated. I'm very strongly against using exceptions as part of the nominal flow of the program.
I much prefer the exists check. Attaching a patch with following changes
1) Not null check for summary 2) In case of exception making the promotion necessary.
Amar Kamat made changes - 04/Apr/08 07:51 AM
Amar Kamat made changes - 04/Apr/08 07:52 AM
Amar Kamat made changes - 04/Apr/08 07:52 AM
One unnecessary import statement slipped in. This patch just removes that.
Amar Kamat made changes - 04/Apr/08 12:19 PM
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12379382/HADOOP-3140-v3.patch against trunk revision 643282. @author +1. The patch does not contain any @author tags. tests included -1. The patch doesn't appear to include any new or modified tests. javadoc +1. The javadoc tool did not generate any warning messages. javac +1. The applied patch does not generate any new javac compiler warnings. release audit +1. The applied patch does not generate any new release audit warnings. findbugs +1. The patch does not introduce any new Findbugs warnings. core tests +1. The patch passed core unit tests. contrib tests +1. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2162/testReport/ This message is automatically generated.
I just committed this. Thanks, Amar!
Devaraj Das made changes - 04/Apr/08 04:34 PM
Integrated in Hadoop-trunk #451 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/451/
)
Nigel Daley made changes - 21/May/08 08:06 PM
Owen O'Malley made changes - 08/Jul/09 04:52 PM
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
JobTracker should not try to promote a (map) task if it does not write to DFS at all
However, there is currently no way to check if the maps wrote side-files to HDFS, in which case we either need a new api for tasks (or jobs) to tell whether they are writing side-files and hence they need promotion or worse, we need to look into the _${taskid} directories and try and guess. Both seem unsatisfactory ...