hadoop-mapreduce-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel Templeton (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (MAPREDUCE-6627) Add machine-readable output to mapred job -history command
Date Thu, 11 Feb 2016 21:59:18 GMT

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

Daniel Templeton commented on MAPREDUCE-6627:
---------------------------------------------

Thanks for the patch, [~rkanter].  A couple of first pass comments:

* Since you're touching the javadocs, the param and return descriptions shouldn't start with
capital letters. It would also be nice to have description fields for the throws tags.
* In the new {{HistoryViewer}} constructor you have:

{code}
    } catch (RuntimeException e) {
      throw e;
    } ...
{code}

* Please add javadoc headers for the {{JobHistoryViewerHumanPrinter}} and {{JobHistoryViewerJsonPrinter}}
classes
* On a philosophical note, I don't like the {{if x then return}} idiom. I'd rather have the
code wrapped in an {{if !x}}.
* I really don't love the nested ternary operators in the comparators in {{JobHistoryViewerHumanPrinter}}
* In the {{JobHistoryViewerJsonPrinter.print()}} method, you have:

{code}
    } catch (JSONException je) {
      throw new IOException(je);
    } finally {
{code}

I'd rather see the IOException have a message that sets some useful context, e.g. {{throw
new IOException("Failure parsing JSON document: " + je, je)}}
* In {{JobHistoryViewerJsonPrinter.print()}}, I'd rather that this:
{code}
  private String fixGroupNameForShuffleErrors(String name) {
    if (name.equals("Shuffle Errors")) {
      return "org.apache.hadoop.mapreduce.task.reduce.Fetcher.ShuffleErrors";
    }
    return name;
  }
{code}
were this:
{code}
  private String fixGroupNameForShuffleErrors(String name) {
    String retName = name;

    if (name.equals("Shuffle Errors")) {
      retName = "org.apache.hadoop.mapreduce.task.reduce.Fetcher.ShuffleErrors";
    }

    return retName;
  }
{code}
But maybe I'm just obsessive.
* I'm not sure the utility you get from {{JobHistoryViewerPrinter}} being an abstract class
is worth it.  I would think hard about making it an interface.
* In {{CLI}}, I'd rather have space around operators, so {{index + 1}} instead of {{index+1}}.
* The tests seem really light.  I'd like to see the tests dig into the JSON object and confirm
that the data is an expected.  I'd also like to see some testing of failure scenarios, like
{{-format biteMe}}.

I haven't applied the patch or played with it yet, though, so there may be more to say later.
:)

> Add machine-readable output to mapred job -history command
> ----------------------------------------------------------
>
>                 Key: MAPREDUCE-6627
>                 URL: https://issues.apache.org/jira/browse/MAPREDUCE-6627
>             Project: Hadoop Map/Reduce
>          Issue Type: Improvement
>          Components: client
>    Affects Versions: 2.9.0
>            Reporter: Robert Kanter
>            Assignee: Robert Kanter
>         Attachments: MAPREDUCE-6627.001.patch, MAPREDUCE-6627.002.patch, json.txt, json_all.txt
>
>
> It would be great if we could add a machine-readable output format, say JSON, to the
{{mapred job -history \[all\] <jobHistoryFile>}} command so that it's easier for programs
to consume that information and do further processing on it.  At the same time, we should
keep the existing API and formatting intact for backwards compatibility.



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

Mime
View raw message