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] Created: (HADOOP-3558) Class Job needs more synchronization
Date Fri, 13 Jun 2008 16:57:44 GMT
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

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

  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