> On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java, > > line 470 > > <https://reviews.apache.org/r/3059/diff/1/?file=62926#file62926line470> > > > > 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.
ok > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java, > > line 471 > > <https://reviews.apache.org/r/3059/diff/1/?file=62926#file62926line471> > > > > what happen if it doesn't exist If it doesnt exist, then the action has no associated stats stored with it. Will add a comment > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java, > > line 486 > > <https://reviews.apache.org/r/3059/diff/1/?file=62926#file62926line486> > > > > remove extra tabs removed in 2nd diff > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java, > > line 562 > > <https://reviews.apache.org/r/3059/diff/1/?file=62926#file62926line562> > > > > use constant .. specially this has been used more than once in this file ok > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/OoziePigStats.java, > > line 45 > > <https://reviews.apache.org/r/3059/diff/1/?file=62927#file62927line45> > > > > do not suppress "unchecked" unless you really need it I need it because JSONObject doesn't support generics > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/OoziePigStats.java, > > line 79 > > <https://reviews.apache.org/r/3059/diff/1/?file=62927#file62927line79> > > > > same as before putting the separator in the loop doesn't seem right > > specially the first element will not be separated done in 2nd diff > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/OoziePigStats.java, > > line 88 > > <https://reviews.apache.org/r/3059/diff/1/?file=62927#file62927line88> > > > > i do not see any type casting here, why you suppress "unchecked" it is there > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java, line > > 47 > > <https://reviews.apache.org/r/3059/diff/1/?file=62929#file62929line47> > > > > 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. Good point..will do > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java, line > > 222 > > <https://reviews.apache.org/r/3059/diff/1/?file=62929#file62929line222> > > > > not clear to me if the comment reflect the reality. did you check for > > the pig version here or for the embedded python will put a better comment > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java, line > > 263 > > <https://reviews.apache.org/r/3059/diff/1/?file=62929#file62929line263> > > > > extra line ok > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java, line > > 341 > > <https://reviews.apache.org/r/3059/diff/1/?file=62929#file62929line341> > > > > 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 As it is an IOException, I am throwing it and will make the laucher handle it. > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java, line > > 364 > > <https://reviews.apache.org/r/3059/diff/1/?file=62929#file62929line364> > > > > make the 100 constant or configurable .. hard coding is not always a > > good programming practice ok > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/resources/oozie-default.xml, line 1107 > > <https://reviews.apache.org/r/3059/diff/1/?file=62930#file62930line1107> > > > > you may add what -1 means here [unlimited] ok > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigMain.java, line > > 294 > > <https://reviews.apache.org/r/3059/diff/1/?file=62929#file62929line294> > > > > check for the existence of the file and for exception > > The file shouldn't not be there in the first place as the temp files from the compute node are deleted once the action ends. If the file, due to some reason, is still there, the Java's FileWriter will overwrite it > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java, > > line 533 > > <https://reviews.apache.org/r/3059/diff/1/?file=62926#file62926line533> > > > > 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 good observation, will put in separate methods > On 2011-12-08 19:33:53, Mohamed Battisha wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java, > > line 893 > > <https://reviews.apache.org/r/3059/diff/1/?file=62925#file62925line893> > > > > I believe you need to handle the HadoopAccessorException here This exception is converted to ActionExecutorException after preserving the the exception cause and message. So, I dont feel the need to handle it separately. - Virag ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3059/#review3708 ----------------------------------------------------------- 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 > >
