----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3059/#review3708 -----------------------------------------------------------
trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java <https://reviews.apache.org/r/3059/#comment8447> I believe you need to handle the HadoopAccessorException here trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3059/#comment8450> use a defined constant for "oozie.action.stats.properties" it seems it has been used for some properties [CONF_OOZIE_EXTERNAL_STATS_MAX_SIZE] but not for others. trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3059/#comment8448> what happen if it doesn't exist trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3059/#comment8451> same as before ... trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3059/#comment8301> remove extra tabs trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3059/#comment8452> The map method seems very long which make it hard to read and increase its error prune, specially with a catch all at the end. it may be better to add you new code in a sperate methods instead of increasing the size of the method trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3059/#comment8453> use constant .. specially this has been used more than once in this file trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3059/#comment8454> same as before trunk/core/src/main/java/org/apache/oozie/action/hadoop/OoziePigStats.java <https://reviews.apache.org/r/3059/#comment8459> do not suppress "unchecked" unless you really need it trunk/core/src/main/java/org/apache/oozie/action/hadoop/OoziePigStats.java <https://reviews.apache.org/r/3059/#comment8455> same as before putting the separator in the loop doesn't seem right specially the first element will not be separated trunk/core/src/main/java/org/apache/oozie/action/hadoop/OoziePigStats.java <https://reviews.apache.org/r/3059/#comment8456> i do not see any type casting here, why you suppress "unchecked" trunk/core/src/main/java/org/apache/oozie/action/hadoop/OoziePigStats.java <https://reviews.apache.org/r/3059/#comment8457> same as before trunk/core/src/main/java/org/apache/oozie/action/hadoop/OoziePigStats.java <https://reviews.apache.org/r/3059/#comment8458> same as before trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java <https://reviews.apache.org/r/3059/#comment8299> Another practice that is used in different pieces of code is to define the prefix [in this case it will be "oozie.action" as a constant and reuse it in the three constant. In this case if the prefex has been changed, there is not need to change all of them. trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java <https://reviews.apache.org/r/3059/#comment8434> not clear to me if the comment reflect the reality. did you check for the pig version here or for the embedded python trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java <https://reviews.apache.org/r/3059/#comment8398> extra line trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java <https://reviews.apache.org/r/3059/#comment8437> check for the existence of the file and for exception trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java <https://reviews.apache.org/r/3059/#comment8441> same as before or more the creation of the file to writeExternalData and make all the validation ther trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java <https://reviews.apache.org/r/3059/#comment8435> it looks odd to have try, finally block without catching any exception. Do you need to handle exception here Especially if you were not able to write to a file.. at least you have to log the error here trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java <https://reviews.apache.org/r/3059/#comment8442> make the 100 constant or configurable .. hard coding is not always a good programming practice trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java <https://reviews.apache.org/r/3059/#comment8446> setting the value of the separator inside the for loop seems unnecessary. it seems also you are not separating the first jobId. trunk/core/src/main/resources/oozie-default.xml <https://reviews.apache.org/r/3059/#comment8397> you may add what -1 means here [unlimited] - Mohamed On 2011-12-08 08:54:20, Virag Kothari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3059/ > ----------------------------------------------------------- > > (Updated 2011-12-08 08:54:20) > > > Review request for oozie, Mohammad Islam and Angelo K. Huang. > > > Summary > ------- > > This patch and https://reviews.apache.org/r/3047/diff addresses Oozie-588 > > > Docs for how to access pig stats through el functions needs to be added > > > This addresses bug Oozie-588. > https://issues.apache.org/jira/browse/Oozie-588 > > > Diffs > ----- > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestPigMain.java > 1211573 > trunk/core/src/test/java/org/apache/oozie/action/hadoop/PigTestCase.java > 1211573 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java > 1211573 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestPigActionExecutor.java > 1211573 > trunk/core/src/main/resources/oozie-default.xml 1211573 > trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java > 1211573 > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigActionExecutor.java > 1211573 > trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java > 1211573 > trunk/core/src/main/java/org/apache/oozie/action/hadoop/OoziePigStats.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > 1211573 > > Diff: https://reviews.apache.org/r/3059/diff > > > Testing > ------- > > Test cases for PigMain and PigAE added > Tested pig action after running examples > > > Thanks, > > Virag > >
