----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3756/#review4883 -----------------------------------------------------------
Comments on consistency in coding, more helpful error messages and test coverage. trunk/client/src/main/java/org/apache/oozie/cli/OozieCLI.java <https://reviews.apache.org/r/3756/#comment10784> The message is very terse without any information on the ability to specify a list. I noticed that the twiki has the details. I am expecting few people to refer to the twiki and more people will refer to the command line message. Can we improve on the message here? trunk/client/src/main/java/org/apache/oozie/client/OozieClient.java <https://reviews.apache.org/r/3756/#comment10757> Shouldn't the list of arguments be (jobId, null, 0, 0) ? trunk/client/src/main/java/org/apache/oozie/client/OozieClient.java <https://reviews.apache.org/r/3756/#comment10758> Shouldn't this be (jobId, filter, start, len) ? trunk/core/src/main/java/org/apache/oozie/BundleEngine.java <https://reviews.apache.org/r/3756/#comment10759> Why are you using filterStr instead of filter - its not consistent with your usage in other methods. trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/3756/#comment10760> Why are you using filterStr instead of filter - its not consistent with your usage in other methods. trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/3756/#comment10761> Can you make this filterList - between filterStr and filter - there is no way to figure out the data types trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/3756/#comment10774> Can you add detailed comments on the expectation of this method - there are several assumptions here in the code that need to be called out. trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/3756/#comment10762> Can you include the pair to help the user with the remedial measure? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/3756/#comment10764> What are the valid values? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/3756/#comment10765> Is status the only filter? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/3756/#comment10766> What are the valid filter keys? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/3756/#comment10767> What happens if the token does not contain "=" ? The error handling is not robust trunk/core/src/main/java/org/apache/oozie/command/coord/CoordJobXCommand.java <https://reviews.apache.org/r/3756/#comment10770> Can you change this to filterList? trunk/core/src/main/java/org/apache/oozie/command/coord/CoordJobXCommand.java <https://reviews.apache.org/r/3756/#comment10771> Can you change this to filterList? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSubsetJPAExecutor.java <https://reviews.apache.org/r/3756/#comment10773> Can you change this to filterList? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSubsetJPAExecutor.java <https://reviews.apache.org/r/3756/#comment10772> Can you change this to filterList? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSubsetJPAExecutor.java <https://reviews.apache.org/r/3756/#comment10778> Add a brief note about the method and change the name to getStatusClause or something more meaningful trunk/core/src/test/java/org/apache/oozie/TestCoordinatorEngine.java <https://reviews.apache.org/r/3756/#comment10781> Can you add a check for the following too: 1. empty filter list (if possible) 2. filter list with the '=' missing 3. filter list with nothing after '=' trunk/core/src/test/java/org/apache/oozie/TestCoordinatorEngine.java <https://reviews.apache.org/r/3756/#comment10779> Can you separate the checks for error code and error message? trunk/core/src/test/java/org/apache/oozie/TestCoordinatorEngine.java <https://reviews.apache.org/r/3756/#comment10780> Can you separate the checks for error code and error message? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsSubsetJPAExecutor.java <https://reviews.apache.org/r/3756/#comment10782> Can you please use more descriptive variable names - for example, filterList? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsSubsetJPAExecutor.java <https://reviews.apache.org/r/3756/#comment10783> What is the advantage of having this private method? Can it be reused by other methods? If the method is retained, please add comments on what is expected of this method? trunk/docs/src/site/twiki/DG_CommandLineTool.twiki <https://reviews.apache.org/r/3756/#comment10785> This line is too technical for an application developer. Can it be simplified to state that when multiple filters are specified, all coordinator actions that satisfy any one of the filters will be retrieved? - Santhosh On 2012-02-03 18:45:55, Virag Kothari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3756/ > ----------------------------------------------------------- > > (Updated 2012-02-03 18:45:55) > > > Review request for oozie. > > > Summary > ------- > > https://issues.apache.org/jira/browse/OOZIE-677 > > > This addresses bug oozie-677. > https://issues.apache.org/jira/browse/oozie-677 > > > Diffs > ----- > > trunk/client/src/main/java/org/apache/oozie/cli/OozieCLI.java 1240247 > trunk/client/src/main/java/org/apache/oozie/client/OozieClient.java 1240247 > trunk/client/src/main/java/org/apache/oozie/client/rest/RestConstants.java > 1240247 > trunk/core/src/main/java/org/apache/oozie/BaseEngine.java 1240247 > trunk/core/src/main/java/org/apache/oozie/BundleEngine.java 1240247 > trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java 1240247 > trunk/core/src/main/java/org/apache/oozie/DagEngine.java 1240247 > trunk/core/src/main/java/org/apache/oozie/ErrorCode.java 1240247 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordJobXCommand.java > 1240247 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSubsetJPAExecutor.java > 1240247 > trunk/core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 1240247 > trunk/core/src/test/java/org/apache/oozie/TestCoordinatorEngine.java > 1240247 > > trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsSubsetJPAExecutor.java > 1240247 > > trunk/core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java > 1240247 > trunk/docs/src/site/twiki/DG_CommandLineTool.twiki 1240247 > > Diff: https://reviews.apache.org/r/3756/diff > > > Testing > ------- > > Test cases added > Checked filter option thru. command line after running cron example > > > Thanks, > > Virag > >
