----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2573/#review2971 -----------------------------------------------------------
trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordActionUpdateStatusJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6646> Can you add some more details here? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordActionUpdateStatusJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6644> Can you please add a comment there that the return type of an update query is Void? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordActionUpdateStatusJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6645> Can you add a comment here that you are returning null since the return type is Void? Can we do away with not returning null? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6652> ...not completed for a given ... trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6667> Can this variable initialization be moved inside the try block? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6651> Need documentation on the mapping. This will aid developers in not spending time looking at the code to figure out the mapping. trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6647> Can id be null? What is the expected outcome if id is null? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6648> Can status be null? What is the error handling case here? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6649> Can pending be null? Is it safe to have a default value of 0 as per the initialization code in CoordinatorActionBean here? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6650> Can the external id be null? Do you need error handling code here? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsRunningJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6653> ... for a given ... trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsRunningJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6666> Can this variable initialization be moved inside the try block? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsRunningJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6654> Need documentation on the mapping. This will aid developers in not spending time looking at the code to figure out the mapping. trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsRunningJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6655> Can id be null? What is the expected outcome if id is null? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsRunningJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6656> Can status be null? What is the error handling case here? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsRunningJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6657> Can pending be null? Is it safe to have a default value of 0 as per the initialization code in CoordinatorActionBean here? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsRunningJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6658> Can the external id be null? Do you need error handling code here? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSuspendedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6659> ... for a given ... trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSuspendedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6665> Can this variable initialization be moved inside the try block? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSuspendedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6660> Need documentation on the mapping. This will aid developers in not spending time looking at the code to figure out the mapping. trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSuspendedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6661> Can id be null? What is the expected outcome if id is null? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSuspendedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6662> Can status be null? What is the error handling case here? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSuspendedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6663> Can pending be null? Is it safe to have a default value of 0 as per the initialization code in CoordinatorActionBean here? trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSuspendedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6664> Can the external id be null? Do you need error handling code here? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6671> If you feel that there is repetition of code for creating the job and the action(s), please move it into a utility class that can be used across many test cases. trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6681> Can you add a comment here about what you are doing next? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6670> How about SUSPENDED, SUCCEEDED and TIMEDOUT? That will complete your testing trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6668> Can this be moved to a separate test case? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsNotCompletedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6669> How about a check for the number of actions? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsRunningJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6680> Can you add a comment here about what you are doing next? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsRunningJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6672> How about SUSPENDED, SUCCEEDED and TIMEDOUT? That will complete your testing trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsRunningJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6673> Can this be moved to a separate test case? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsStatusByPendingFalseJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6679> Can you add a comment here about what you are doing next? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsStatusByPendingFalseJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6676> How about SUSPENDED, SUCCEEDED and TIMEDOUT? That will complete your testing trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsStatusByPendingFalseJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6674> Can this be moved to a separate test case? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsSuspendedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6678> Can you add a comment here about what you are doing next? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsSuspendedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6677> How about SUSPENDED, SUCCEEDED and TIMEDOUT? That will complete your testing trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsSuspendedJPAExecutor.java <https://reviews.apache.org/r/2573/#comment6675> Can this be moved to a separate test case? - Santhosh On 2011-10-31 16:58:25, Virag Kothari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2573/ > ----------------------------------------------------------- > > (Updated 2011-10-31 16:58:25) > > > Review request for oozie, Mohammad Islam and Angelo K. Huang. > > > Summary > ------- > > Bug oozie-587 > Summary: https://issues.apache.org/jira/browse/OOZIE-587 > > > This addresses bug OOZIE-587. > https://issues.apache.org/jira/browse/OOZIE-587 > > > Diffs > ----- > > trunk/core/src/main/java/org/apache/oozie/CoordinatorActionBean.java > 1189491 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionCheckXCommand.java > 1189491 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordKillXCommand.java > 1189491 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordResumeXCommand.java > 1189491 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordSuspendXCommand.java > 1189491 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordActionGetJPAExecutor.java > 1189491 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordActionUpdateStatusJPAExecutor.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsNotCompletedJPAExecutor.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsRunningJPAExecutor.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsStatusByPendingFalseJPAExecutor.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsSuspendedJPAExecutor.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetPendingActionsCountJPAExecutor.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/service/StatusTransitService.java > 1189491 > > trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordActionUpdateStatusJPAExecutor.java > PRE-CREATION > > trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsNotCompletedJPAExecutor.java > PRE-CREATION > > trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsRunningJPAExecutor.java > PRE-CREATION > > trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsStatusByPendingFalseJPAExecutor.java > PRE-CREATION > > trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsSuspendedJPAExecutor.java > PRE-CREATION > > trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetPendingActionsCountJPAExecutor.java > PRE-CREATION > trunk/release-log.txt 1189491 > > Diff: https://reviews.apache.org/r/2573/diff > > > Testing > ------- > > Added test cases for JPAExecutor. Tested against current test cases. > > > Thanks, > > Virag > >
