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

["Dick King (JIRA)" <ji...@apache.org>]

    [ 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.