Mailing-List: contact mapreduce-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: mapreduce-issues@hadoop.apache.org
Date: Thu, 11 Feb 2016 21:59:18 +0000 (UTC)
From: "Daniel Templeton (JIRA)" <jira@apache.org>
To: mapreduce-issues@hadoop.apache.org
Message-ID: <JIRA.12936568.1454549082000.30587.1455227958220@Atlassian.JIRA>
In-Reply-To: <JIRA.12936568.1454549082000@Atlassian.JIRA>
References: <JIRA.12936568.1454549082000@Atlassian.JIRA>
 <JIRA.12936568.1454549082989@arcas>
Subject: [jira] [Commented] (MAPREDUCE-6627) Add machine-readable output to
 mapred job -history command
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


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