hadoop-common-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aaron Greenhouse (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HADOOP-3558) Class Job needs more synchronization
Date Mon, 16 Jun 2008 16:35:47 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-3558?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12605341#action_12605341

Aaron Greenhouse commented on HADOOP-3558:

Per Nigel Daley's request, I should point out that this bug was discovered using Sierra from
SureLogic to view FindBug results. The bug was corrected with the assistance of JSure from

> Class Job needs more synchronization
> ------------------------------------
>                 Key: HADOOP-3558
>                 URL: https://issues.apache.org/jira/browse/HADOOP-3558
>             Project: Hadoop Core
>          Issue Type: Bug
>    Affects Versions: 0.17.0
>            Reporter: Aaron Greenhouse
>         Attachments: Job.patch
>   Original Estimate: 1h
>  Remaining Estimate: 1h
> Class Job needs additional synchronization to be truly thread-safe.  At a minimum, the
following changes should be made:
> * Making the field jc final.
> * Adding the synchronized modifier to the methods toString(), getJobName(), setJobName(),
getJobID(), setJobID(), getMapredJobID(), setMapredJobID(), getJobConf(), setJobConf(), getMessage(),
setMessage(), getDependingJobs(), isCompleted(), and isReady(). 
> * Fix the method getDependingJobs() to return a *copy* of the list:
>   public synchronized ArrayList<Job> getDependingJobs() {
>      return new ArrayList<Job>(this.dependingJobs);
>    }
> The class can be further improved, however:
> * The methods setJobID() and setState() should be made package private (i.e., default
visibility) so that they can only be called from the JobControl class.
> * Reconsider the necessity of the all the getter and setter methods. In particular, I
question the need for getJobConf() and setJobConf(), getDependingJobs(), setJobName(), setMapredJobId(),
and setMesage(). If these methods were eliminated, then the fields theJobConf and jobName
could be made final.
> * The field dependingJobs could also be made final if the constructor Job(JobConf) were
changed to
>   public Job(JobConf jobConf) throws IOException {
>      this(jobConf, new ArrayList());
>    }
> Then addDependingJob() could be simplified to
>   public synchronized boolean addDependingJob(Job dependingJob) {
>      if (this.state == Job.WAITING) { //only allowed to add jobs when waiting
>        return this.dependingJobs.add(dependingJob);
>      } else {
>        return false;
>      }
>    }
> This class has the additional weakness that the list object referenced by field dependingJobs
is provided by the outside environment to the constructor.  This can be eliminated by copying
the list when the object is constructed:
>   public Job(JobConf jobConf, ArrayList<Job> dependingJobs) throws IOException
>     ...
>     this.dependingJobs = new ArrayList<Job>(dependingJobs);
>     ...
>   }

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

View raw message