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

Philip Zeyliger commented on MAPREDUCE-777:
-------------------------------------------

Took a quick pass at your patch.  Some comments, mostly documentation-related.

bq. +  static Counters downgrade(org.apache.hadoop.mapreduce.Counters counters) 
{

You might have some JavaDoc for this method.  Also, variables would be clearer 
if everything were old_counter and new_counter, since it's hard to keep track 
what's what.

bq. ClientProtocol

Are we settled on the name ClientProtocol?  It's quite generic sounding, and, 
without the package, hard to decipher.  Since these protocols will be the names 
of the public-ish wire APIs, perhaps JobClientProtocol would be more 
descriptive?

bq. +public class CLI extends Configured implements Tool {

Some of Hadoop uses apache.commons.cli to parse command line arguments.  (And 
there's CLI2 too, referred to in Maven, though I don't see any usages of it.  
You might consider using a command-line parsing library.

You might also consider splitting up the run() method into separate methods 
(even classes) for each piece of functionality.  This will make it much easier 
to test, and easier to parse, too.


bq. +public interface ClientProtocol extends VersionedProtocol {

In the javadoc here documenting the history of this protocol, you might mention 
the rename. 

bq. "Changed protocol to use new api"

This is not very descriptive for someone unfamiliar with this ticket.


Cheers,

-- Philip

> A method for finding and tracking jobs from the new API
> -------------------------------------------------------
>
>                 Key: MAPREDUCE-777
>                 URL: https://issues.apache.org/jira/browse/MAPREDUCE-777
>             Project: Hadoop Map/Reduce
>          Issue Type: New Feature
>          Components: client
>            Reporter: Owen O'Malley
>            Assignee: Amareshwari Sriramadasu
>             Fix For: 0.21.0
>
>         Attachments: m-777.patch, patch-777-1.txt, patch-777-2.txt, 
> patch-777-3.txt, patch-777.txt
>
>
> We need to create a replacement interface for the JobClient API in the new 
> interface. In particular, the user needs to be able to query and track jobs 
> that were launched by other processes.

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

Reply via email to