[jira] Commented: (MAPREDUCE-1118) Capacity Scheduler scheduling information is hard to read / should be tabular format

[Dick King (JIRA)]

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

Dick King commented on MAPREDUCE-1118:
--------------------------------------

This comment is a review.

First, let me say that I didn't review {{sorttable.js}} .  It would be bad to 
have subtly different versions of this code flying around.

{{CapacitySchedulerServlet.java}} 

near end of {{doGet()}} :

*This is serious*:  {{ByteArrayOutputSteam.writeTo(OutputStream)}} throws.  
Please revise this call to something like

{noformat}
  OutputStream servletOut = null;
  try {
    servletOut = response.getOutputStream();
    baos.writeTo(servletOut);
  } finally {
    if (servletOut != null) {
      servletOut.close();
    }
  }
{noformat}

.

*This is semi-serious*:  In {{showQueues}} , where queues are printed, the code

{noformat}
  out.printf(
        "<td><a href=\"jobqueue_details.jsp?queueName=%s\">%s</a></td>\n",
        name, name);
{noformat}

the code deposits the name right in the middle of hard-core HTML.  If the queue 
names contain obnoxious characters such as a quote or an angle bracket we could 
have a bad day.  These characters should be escaped with HTML escape sequences 
such as {{&lt;}} , etc.

Don't forget to escape the ampersands :-) .  I believe that only quote marks 
and angle brackets need to be escaped in the URL, but everything needs to be 
escaped in the rendered text.

*This is a nit*:  In

{noformat}
   out.printf("<td>%s</td>\n", queuesManager.getJobQueue(name)
        .getRunningJobs().size());
   out.printf("<td>%s</td>\n", qsc.getNumOfWaitingJobs());
{noformat}

I can't condone dropping numeric data onto a {{%s}} .  I realize that it works 
but it looks ugly to my eye.

*This is potentially serious*: I don't see where {{showQueues}} does the needed 
locking.  You allude to this by defensively dumping into a 
{{ByteArrayOutputStream}} , but the code doesn't lock anything.  I can see why 
it should.  Can queues disappear or appear?

*This is a potential omission*: The block comment before the {{class}} 
declaration claims to implement an advanced mode, but I don't see any footprint 
of such a thing in the code.  In any event, I'm not a big fan of magic URLs.  
The servlet should include a button to bring itself into advanced mode.  If 
there are users that shouldn't be able to go into advanced mode, this should be 
handled in some other manner than hidden URLs.

I don't see the code to get into the scheduler manager servlet.  Perhaps there 
should be a button in the job tracker administration page when the capacity 
scheduler is in use?

{{TaskSchedulingMgr}}

{{infoServer.setAttribute("scheduler", this);}}

*This is a nit*: I would prefer 
{{infoServer.setAttribute("scheduler.scheduler", this);}} .  All of the 
servlets share an attribute namespace.  However, this one isn't bad as such 
things go, since it's hard to imagine another servlet code author putting 
anything except the ambient scheduler into that attribute.

{{TestCapacitySchedulerServlet}}

This is a minor nit.

I can't condone {{assertTrue(queueData.contains("50.0%"));}} .  That's the 
moral equivalent of floating point equality.  I do realize that 1/2 can be 
represented exactly in most float systems, but you might want to do something 
else, even if only allowing the value to be {{49.9}} which is okay because the 
servlet does print it out as a {{%.1f}} .

> Capacity Scheduler scheduling information is hard to read / should be tabular 
> format
> ------------------------------------------------------------------------------------
>
>                 Key: MAPREDUCE-1118
>                 URL: https://issues.apache.org/jira/browse/MAPREDUCE-1118
>             Project: Hadoop Map/Reduce
>          Issue Type: Bug
>    Affects Versions: 0.20.2
>            Reporter: Allen Wittenauer
>            Assignee: Krishna Ramachandran
>         Attachments: mapred-1118-1.patch, mapred-1118-2.patch, 
> mapred-1118.20S.patch, mapred-1118.patch
>
>
> The scheduling information provided by the capacity scheduler is extremely 
> hard to read on the job tracker web page.  Instead of just flat text, it 
> should be presenting the information in a tabular format, similar to what the 
> fair share scheduler provides.  This makes it much easier to compare what 
> different queues are doing.

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