----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3047/#review3711 -----------------------------------------------------------
trunk/client/src/main/java/org/apache/oozie/client/rest/JsonTags.java <https://reviews.apache.org/r/3047/#comment8305> Rename "externalchildids" to "externalChildIds". Rest of the variable values appear to be following the later format. trunk/core/src/main/java/org/apache/oozie/DagELFunctions.java <https://reviews.apache.org/r/3047/#comment8307> Specific reason for the trailing ':' in value of HADOOP_JOBS_PREFIX? trunk/core/src/main/java/org/apache/oozie/DagELFunctions.java <https://reviews.apache.org/r/3047/#comment8308> 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? trunk/core/src/main/java/org/apache/oozie/action/hadoop/ActionStats.java <https://reviews.apache.org/r/3047/#comment8312> 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. trunk/core/src/main/java/org/apache/oozie/action/hadoop/ActionStats.java <https://reviews.apache.org/r/3047/#comment8310> Parent class should not know the type of derived classes. setCurrentActionType is being called only from derived classes. This method can be just removed. trunk/core/src/main/java/org/apache/oozie/action/hadoop/MRStats.java <https://reviews.apache.org/r/3047/#comment8317> 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. trunk/core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java <https://reviews.apache.org/r/3047/#comment8321> Link to the comment above regarding ActionType parameter to MRStats ctor being redundant. trunk/core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java <https://reviews.apache.org/r/3047/#comment8322> Remove <<Params>> :) trunk/core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java <https://reviews.apache.org/r/3047/#comment8324> [Styling related comment] Alternative can be: if (actionConf == null) { return ""; } // code right now inside if // Less indentation -> More readability trunk/core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java <https://reviews.apache.org/r/3047/#comment8325> [Cosmetic comment] Compiler will most likely remove local variables 'name' and 'value'. You can also :) trunk/core/src/main/java/org/apache/oozie/client/rest/JsonWorkflowAction.java <https://reviews.apache.org/r/3047/#comment8326> [Cosmetic comment] Place getStats() next to setStats()? - Prakhar On 2011-12-07 07:06:39, params wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3047/ > ----------------------------------------------------------- > > (Updated 2011-12-07 07:06:39) > > > 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/core/src/test/java/org/apache/oozie/client/rest/TestJsonWorkflowAction.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/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/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/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/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/action/hadoop/MapReduceActionExecutor.java > 1211308 > trunk/core/src/main/java/org/apache/oozie/action/hadoop/MRStats.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/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/WorkflowActionBean.java 1211308 > trunk/core/src/main/java/org/apache/oozie/DagELFunctions.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/client/src/main/java/org/apache/oozie/client/WorkflowAction.java > 1211308 > trunk/client/src/main/java/org/apache/oozie/cli/OozieCLI.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 > >
