> On 2011-12-07 21:29:50, Prakhar Sharma wrote: > > trunk/core/src/main/java/org/apache/oozie/client/rest/JsonWorkflowAction.java, > > line 325 > > <https://reviews.apache.org/r/3047/diff/1/?file=62775#file62775line325> > > > > [Cosmetic comment] > > Place getStats() next to setStats()?
Moved. > On 2011-12-07 21:29:50, Prakhar Sharma wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java, > > line 200 > > <https://reviews.apache.org/r/3047/diff/1/?file=62774#file62774line200> > > > > [Cosmetic comment] > > Compiler will most likely remove local variables 'name' and 'value'. > > You can also :) Changed. > On 2011-12-07 21:29:50, Prakhar Sharma wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java, > > line 158 > > <https://reviews.apache.org/r/3047/diff/1/?file=62774#file62774line158> > > > > Remove <<Params>> :) Removed. > On 2011-12-07 21:29:50, Prakhar Sharma wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java, > > line 152 > > <https://reviews.apache.org/r/3047/diff/1/?file=62774#file62774line152> > > > > Link to the comment above regarding ActionType parameter to MRStats > > ctor being redundant. The first argument is no more required to be passed (ActionType is set implicitly inside the class based on the type of stats). > On 2011-12-07 21:29:50, Prakhar Sharma wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/MRStats.java, line > > 32 > > <https://reviews.apache.org/r/3047/diff/1/?file=62773#file62773line32> > > > > Isn't ActionType parameter to the ctor redundant? Will this class ever > > create an object with value of ActionType parameter other than MAP_REDUCE? > > If yes, then you can ignore this comment. Yes, it seems redundant. Removing it from here. Instead, it ll be set inside the ctor based on the type of action. > On 2011-12-07 21:29:50, Prakhar Sharma wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/ActionStats.java, > > line 39 > > <https://reviews.apache.org/r/3047/diff/1/?file=62771#file62771line39> > > > > Parent class should not know the type of derived classes. > > setCurrentActionType is being called only from derived classes. This method > > can be just removed. Removing the setter. > On 2011-12-07 21:29:50, Prakhar Sharma wrote: > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/ActionStats.java, > > line 25 > > <https://reviews.apache.org/r/3047/diff/1/?file=62771#file62771line25> > > > > Make currentActionType protected and set it directly in derived classes > > and do away with the corresponding setter. > > > > Even better design will be not to have the currentActionType in parent > > class altogether and have an abstract method (like toJSON()) which returns > > the type. Motivation being: Parent should not care who is deriving from it. Made currentActionType protected. Corresponding setter will be removed. > On 2011-12-07 21:29:50, Prakhar Sharma wrote: > > trunk/core/src/main/java/org/apache/oozie/DagELFunctions.java, line 116 > > <https://reviews.apache.org/r/3047/diff/1/?file=62768#file62768line116> > > > > Rational behind short circuit condition: action.getData() == null > > > > Inside the if block you are not accessing return value of > > action.getData(). Looks like you might end up missing some cases due to the > > check. > > > > If ensuring (action.getData() != null && action.getExternalChildIds() > > != null) not being true is the motivation, then may be consider throwing an > > exception? Agreed. This might miss some cases. Removing getData() check from the if condition check. Retaining just the required check alone (which is to test that externalChildIDs are not null). > On 2011-12-07 21:29:50, Prakhar Sharma wrote: > > trunk/client/src/main/java/org/apache/oozie/client/rest/JsonTags.java, line > > 69 > > <https://reviews.apache.org/r/3047/diff/1/?file=62766#file62766line69> > > > > Rename "externalchildids" to "externalChildIds". > > Rest of the variable values appear to be following the later format. Renamed. > On 2011-12-07 21:29:50, Prakhar Sharma wrote: > > trunk/core/src/main/java/org/apache/oozie/DagELFunctions.java, line 42 > > <https://reviews.apache.org/r/3047/diff/1/?file=62768#file62768line42> > > > > Specific reason for the trailing ':' in value of HADOOP_JOBS_PREFIX? Hadoop Job IDs are stored in the WorkflowInstance as a property (hadoopJobs:job1,job2). Since, we are creating this property in the DAGELFunctions.java, we need to prefix "hadoopJobs:" to the data retrieved from getExternalChildIDs() function - ("job1,job2"). For the other variables stored in WorkflowInstance, this prefixing is not required as they are already retrieved as properties (A:a). - params ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3047/#review3711 ----------------------------------------------------------- On 2011-12-07 23:53:52, params wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3047/ > ----------------------------------------------------------- > > (Updated 2011-12-07 23:53:52) > > > Review request for oozie, Mohammad Islam and Angelo K. Huang. > > > Summary > ------- > > OOZIE-588: Oozie to allow drill down to hadoop job's details > > Summary: > https://issues.apache.org/jira/browse/OOZIE-588 > > Features Implemented: > - Expose action stats information (hadoop counters and pig stats) through CLI > -info -verbose option > - Expose external child IDs (eg: hadoop job IDs for the pig action) through > CLI -info -verbose option > - Expose job counter info as EL Functions > - Expose external child IDs as EL Functions > - User level configuration to specify if stats should be collected or not > (with a default setting) > - Oozie level configuration to specify the max allowable size for stats > > > This addresses bug OOZIE-588. > https://issues.apache.org/jira/browse/OOZIE-588 > > > Diffs > ----- > > trunk/client/src/main/java/org/apache/oozie/cli/OozieCLI.java 1211308 > trunk/client/src/main/java/org/apache/oozie/client/WorkflowAction.java > 1211308 > trunk/client/src/main/java/org/apache/oozie/client/rest/JsonTags.java > 1211308 > trunk/client/src/main/java/org/apache/oozie/client/rest/JsonToBean.java > 1211308 > trunk/core/src/main/java/org/apache/oozie/DagELFunctions.java 1211308 > trunk/core/src/main/java/org/apache/oozie/WorkflowActionBean.java 1211308 > trunk/core/src/main/java/org/apache/oozie/action/ActionExecutor.java > 1211308 > trunk/core/src/main/java/org/apache/oozie/action/hadoop/ActionStats.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/action/hadoop/ActionType.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/action/hadoop/MRStats.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java > 1211308 > > trunk/core/src/main/java/org/apache/oozie/client/rest/JsonWorkflowAction.java > 1211308 > trunk/core/src/main/java/org/apache/oozie/command/wf/ActionCommand.java > 1211308 > trunk/core/src/main/java/org/apache/oozie/command/wf/ActionXCommand.java > 1211308 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/WorkflowActionGetJPAExecutor.java > 1211308 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/WorkflowActionSubsetGetJPAExecutor.java > 1211308 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/WorkflowActionsGetForJobJPAExecutor.java > 1211308 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/WorkflowActionsRunningGetJPAExecutor.java > 1211308 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/WorkflowJobGetActionsJPAExecutor.java > 1211308 > trunk/core/src/main/java/org/apache/oozie/store/WorkflowStore.java 1211308 > trunk/core/src/test/java/org/apache/oozie/TestActionBean.java 1211308 > > trunk/core/src/test/java/org/apache/oozie/action/decision/TestDecisionActionExecutor.java > 1211308 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java > 1211308 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestHadoopELFunctions.java > 1211308 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionError.java > 1211308 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java > 1211308 > > trunk/core/src/test/java/org/apache/oozie/action/ssh/TestSshActionExecutor.java > 1211308 > > trunk/core/src/test/java/org/apache/oozie/client/rest/TestJsonWorkflowAction.java > 1211308 > > Diff: https://reviews.apache.org/r/3047/diff > > > Testing > ------- > > - Tested against existing test cases and added tests where required. > - Tests for the new classes MRStats and OoziePigStats have not been added. > This is work in progress. > > > Thanks, > > params > >
