hadoop-mapreduce-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Harsh Chouraria" <qwertyman...@gmail.com>
Subject Re: Review Request: MAPREDUCE-2584. Check for serializers early, and give out more information regarding missing serializers.
Date Sun, 12 Jun 2011 04:36:05 GMT


> On 2011-06-12 02:31:03, Todd Lipcon wrote:
> > src/java/org/apache/hadoop/mapreduce/JobSubmitter.java, line 400
> > <https://reviews.apache.org/r/885/diff/1/?file=21001#file21001line400>
> >
> >     nit: the javadoc style guide says this should be:
> >     @param job the job to check for
> >     
> >     (so it formats nicely)

Done.


> On 2011-06-12 02:31:03, Todd Lipcon wrote:
> > src/java/org/apache/hadoop/mapreduce/JobSubmitter.java, lines 401-403
> > <https://reviews.apache.org/r/885/diff/1/?file=21001#file21001line401>
> >
> >     I'm generally against putting empty @throws in JavaDoc - it doesn't tell you
anything the method signature doesn't already say. I'd just delete these lines

Agreed. It came on by default in Eclipse, and I had let them be instead of filling it up.
Removed.


> On 2011-06-12 02:31:03, Todd Lipcon wrote:
> > src/java/org/apache/hadoop/mapreduce/JobSubmitter.java, lines 417-420
> > <https://reviews.apache.org/r/885/diff/1/?file=21001#file21001line417>
> >
> >     same comments apply

Removed.


> On 2011-06-12 02:31:03, Todd Lipcon wrote:
> > src/java/org/apache/hadoop/mapreduce/JobSubmitter.java, lines 439-440
> > <https://reviews.apache.org/r/885/diff/1/?file=21001#file21001line439>
> >
> >     rather than this "note" why not just change the above line to "Checks if the
map-output key and value classes have serializers and deserializers associated with them."?

Fixed to a more direct message. Sometimes my commenting language is pretty verbose and confusing,
thanks for correcting!


> On 2011-06-12 02:31:03, Todd Lipcon wrote:
> > src/java/org/apache/hadoop/mapreduce/JobSubmitter.java, line 470
> > <https://reviews.apache.org/r/885/diff/1/?file=21001#file21001line470>
> >
> >     missing a space in this exception method.

Added.


> On 2011-06-12 02:31:03, Todd Lipcon wrote:
> > src/test/mapred/org/apache/hadoop/mapreduce/TestMRJobClient.java, line 87
> > <https://reviews.apache.org/r/885/diff/1/?file=21002#file21002line87>
> >
> >     add an assert that e.getMessage().contains("Couldn't find a serializer") perhaps?

Added as well.


- Harsh


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/885/#review806
-----------------------------------------------------------


On 2011-06-11 22:16:24, Harsh Chouraria wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/885/
> -----------------------------------------------------------
> 
> (Updated 2011-06-11 22:16:24)
> 
> 
> Review request for hadoop-mapreduce.
> 
> 
> Summary
> -------
> 
> As discussed on HADOOP-7328, MapReduce can handle serializers in a much better way in
case of bad configuration, improper imports (Some odd Text class instead of the Writable Text
set as key), etc..
> 
> This issue covers the MapReduce parts of the improvements (made to MapOutputBuffer and
possible early-check of serializer availability pre-submit) that provide more information
than just an NPE as is the current case.
> 
> 
> This addresses bug MAPREDUCE-2584.
>     http://issues.apache.org/jira/browse/MAPREDUCE-2584
> 
> 
> Diffs
> -----
> 
>   src/java/org/apache/hadoop/mapred/MapTask.java 21599c2 
>   src/java/org/apache/hadoop/mapreduce/JobSubmitter.java 751d528 
>   src/test/mapred/org/apache/hadoop/mapreduce/TestMRJobClient.java 5fa329a 
> 
> Diff: https://reviews.apache.org/r/885/diff
> 
> 
> Testing
> -------
> 
> Added a test case that expects a failure if no io.serializers are present.
> 
> 
> Thanks,
> 
> Harsh
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message