----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1091/#review1109 -----------------------------------------------------------
http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMean.java <https://reviews.apache.org/r/1091/#comment2249> Minor nits: Please remove all the extra whitespaces from empty lines and empty comment lines. If you look at your colorized git diff output, or the diff on reviewboard, you should be able to see where all these are present in bright red :) http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMean.java <https://reviews.apache.org/r/1091/#comment2250> Please reuse the same driver configuration instance, and avoid creating new configurations. Take this as a general practice tip too, its best to work with just one configuration instance for as long as relevant! :) http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMean.java <https://reviews.apache.org/r/1091/#comment2251> This shouldn't happen, and ought to be ensured by the test cases. I'd say its unnecessary here. The IOException handler doesn't add much value to the exception either, and can perhaps be avoided for a general <throws IOException> method? http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMean.java <https://reviews.apache.org/r/1091/#comment2252> Minor nit again, but you could also do GenericOptionsParser(args) first, and then extract the configuration out of it as GenericOptionsParser.getConfiguration(). This, cause you don't seem to be tweaking the original conf instance really. Again, minor nit, feel free to ignore :-) http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMedian.java <https://reviews.apache.org/r/1091/#comment2253> Like in the last class above, you can reuse a single IntWritable instance and keep IntWritable.set()-ing it instead of a new instance per call. http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMedian.java <https://reviews.apache.org/r/1091/#comment2254> Sum can too be made a member var with resets, and also the previous comment applies to Reducer's context write as well. You can utilize a single IntWritable stmt with set(int)-ing it every turn. http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMedian.java <https://reviews.apache.org/r/1091/#comment2255> Please reuse config instance here as well. http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMedian.java <https://reviews.apache.org/r/1091/#comment2256> If a file does not exist, perhaps its best not to return a wrong value but to fire away an exception? http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMedian.java <https://reviews.apache.org/r/1091/#comment2257> ! can be removed :) http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMedian.java <https://reviews.apache.org/r/1091/#comment2258> Same applies here, remove the exclamatory. http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMedian.java <https://reviews.apache.org/r/1091/#comment2259> Also, instead of directly using GenericOptionsParser, have you considered writing it with the Tool class framework? That's how we usually recommend writing submission jars. http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMedian.java <https://reviews.apache.org/r/1091/#comment2260> If this class is public, use its .class.getCanonicalName() method directly via an import itself? Also, I though there were public counter enum groups available. Look for those instead of fetching w/ strings, and if they don't exist then this is fine. - Harsh On 2011-07-18 23:26:12, Plamen Jeliazkov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1091/ > ----------------------------------------------------------- > > (Updated 2011-07-18 23:26:12) > > > Review request for hadoop-mapreduce. > > > Summary > ------- > > Looking to add 3 new examples -- they can be added via the diff attached; > some changes to the ExamplesDriver.java might be required however these files > do work alone as well. I will also be attaching JUnit tests for these > examples. I will post another review request for those. > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/test/mapred/org/apache/hadoop/examples/TestWordStandardDeviation.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/test/mapred/org/apache/hadoop/examples/input/shaks12.txt > PRE-CREATION > > http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMedian.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordStandardDeviation.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/test/mapred/org/apache/hadoop/examples/TestWordMean.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/test/mapred/org/apache/hadoop/examples/TestWordMedian.java > PRE-CREATION > > http://svn.apache.org/repos/asf/hadoop/common/branches/branch-0.22/mapreduce/src/examples/org/apache/hadoop/examples/WordMean.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/1091/diff > > > Testing > ------- > > JUnit tests added -- reduced to 3 tests that all read from an input folder. > > > Thanks, > > Plamen > >
