[ 
https://issues.apache.org/jira/browse/MAPREDUCE-6542?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15022639#comment-15022639
 ] 

Daniel Templeton commented on MAPREDUCE-6542:
---------------------------------------------

[~piaoyu zhang], thanks!  Looks like you have all the rights pieces now.  We 
just have some details to iron out in the test code.

{code}
  static final FastDateFormat fastDateFormat = 
FastDateFormat.getInstance("d-MMM-yyyy HH:mm:ss");
{code}

Let's make this private and move it up to the top of the class.  I find member 
variables declared in the middle of a class to be confusing.

{code}
  @Test
  //Multithreaded Test GetFormattedTimeWithDiff()
  public void testGetFormattedTimeWithDiff() {
    class Worker implements Runnable{
{code}

Please add a space before the open brace.

{code}
      private CyclicBarrier cyclicBarrier;
{code}

Insert a newline here.

{code}
      public Worker(CyclicBarrier cyclicBarrier){
{code}

Please add a space before the open brace.

{code}
        this.cyclicBarrier = cyclicBarrier;
      }
{code}

Insert a newline here.

{code}
      @Override public void run() {
{code}

Please put the {{@Override}} on a line by itself for clarity and consistency.

{code}
        final long currentTime = System.currentTimeMillis() - 30000 ;
        final long currentTime2 = currentTime ;
{code}

I think what you meant to do was set {{currentTime2 = 
System.currentTimeMillis()}} and then set {{currentTime = currentTime2 - 
30000}}.  Also, please take out the spaces before the semicolons.

{code}
        String time1 = StringUtils.getFormattedTimeWithDiff(fastDateFormat, 
currentTime, 0);
        String time2 = StringUtils.getFormattedTimeWithDiff(fastDateFormat, 
currentTime2, 0);
{code}

I think you can just call {{getFormattedTimeWithDiff(fastDateFormat, 
currentTime2, currentTime)}} twice and then compare the results.

{code}
        assertTrue("Method returned consistent results indicative of a race 
condition",
            time1.equals(time2));
{code}

Should be *in*consistent, not consistent.

{code}
        try {
          this.cyclicBarrier.await();
        } catch (Exception e) {
          e.printStackTrace();
        }
{code}

The barrier should go at the beginning of the method instead of at the end.  
With it at the end, it's making sure all threads terminate at the same time, 
but does nothing to synchronize the time formatting.

{code}
      }
    }

    CyclicBarrier cyclicBarrier = new CyclicBarrier(1000);
    for (int i = 0; i < 1000; i++) {
      new Thread(new Worker(cyclicBarrier)).start();
    }
  }
{code}

Instead of creating {{Thread}} objects directly, I liked your previous approach 
of using an {{ExecutorService}} better.

1000 is a lot of threads.  It's highly unlikely that this code will run on a 
system with that level of concurrency.  Instead, I think you'd be better served 
by dropping the thread count to something more like 10 and having each thread 
loop to make up the difference.

If you want to find the number of loops to use empirically, run the code with a 
{{DateFormat}} instead to see how many loops it takes to make it break, and 
then multiply that number by 10 to be safe.

Almost there!

> HistoryViewer use SimpleDateFormat,But SimpleDateFormat is not threadsafe
> -------------------------------------------------------------------------
>
>                 Key: MAPREDUCE-6542
>                 URL: https://issues.apache.org/jira/browse/MAPREDUCE-6542
>             Project: Hadoop Map/Reduce
>          Issue Type: Bug
>          Components: jobhistoryserver
>    Affects Versions: 2.2.0, 2.7.1
>         Environment: CentOS6.5 Hadoop  
>            Reporter: zhangyubiao
>            Assignee: zhangyubiao
>         Attachments: MAPREDUCE-6542-v2.patch, MAPREDUCE-6542-v3.patch, 
> MAPREDUCE-6542-v4.patch, MAPREDUCE-6542-v5.patch, MAPREDUCE-6542.patch
>
>
> I use SimpleDateFormat to Parse the JobHistory File before 
> {code}
> private static final SimpleDateFormat dateFormat =
>     new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
>  public static String getJobDetail(JobInfo job) {
>         StringBuffer jobDetails = new StringBuffer("");
>         SummarizedJob ts = new SummarizedJob(job);
>         jobDetails.append(job.getJobId().toString().trim()).append("\t");
>         jobDetails.append(job.getUsername()).append("\t");
>         jobDetails.append(job.getJobname().replaceAll("\\n", 
> "")).append("\t");
>         jobDetails.append(job.getJobQueueName()).append("\t");
>         jobDetails.append(job.getPriority()).append("\t");
>         jobDetails.append(job.getJobConfPath()).append("\t");
>         jobDetails.append(job.getUberized()).append("\t");
>         
> jobDetails.append(dateFormat.format(job.getSubmitTime())).append("\t");
>         
> jobDetails.append(dateFormat.format(job.getLaunchTime())).append("\t");
>         
> jobDetails.append(dateFormat.format(job.getFinishTime())).append("\t");
>        return jobDetails.toString();
> }
> {code}
> But I find I query the SubmitTime and LaunchTime in hive and compare 
> JobHistory File time , I find that the submitTime  and launchTime was wrong.
> Finally,I change to use the FastDateFormat to parse the time format and the 
> time become right



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to