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