[jira] Updated: (HADOOP-3558) Class Job needs more synchronization

["Konstantin Boudnik (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/HADOOP-3558?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Konstantin Boudnik updated HADOOP-3558:
---------------------------------------

    Tags: surelogic

> Class Job needs more synchronization
> ------------------------------------
>
>                 Key: HADOOP-3558
>                 URL: https://issues.apache.org/jira/browse/HADOOP-3558
>             Project: Hadoop Common
>          Issue Type: Bug
>    Affects Versions: 0.17.0
>            Reporter: Aaron Greenhouse
>         Attachments: HADOOP-3558.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.