----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2948/#review3549 -----------------------------------------------------------
Overall comments: Stick to Java coding convention regarding for if, else, else if Need comments to describe huge chunks of code The test coverage requires negative test cases for error handling and the three cases where you check for isEnabled and isUnit trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7886> Can you make it case insensitive? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7887> Please use NANException. NAN is well defined. trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7888> The error message has to be more informative. Why is the value for frequency invalid? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7889> Can you please make it case insensitive? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7890> Can you please make it case insensitive? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7891> Can you list the valid values for unit? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7894> Can you add a comment describing the logic for the big block of code? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7892> Can you rephrase the message to "frequency must be specified with time unit" trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7893> Move the else to the same line as the closing brace as per Java coding convention trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7895> Can you please make it case insensitive? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7901> Move the else to this line as per Java coding convention trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7896> Can you please make it case insensitive? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7900> Move the else to this line as per Java coding convention trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7897> Can you please make it case insensitive? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7899> Move the else to this line as per Java coding convention trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7898> Can you please make it case insensitive? trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java <https://reviews.apache.org/r/2948/#comment7902> Can you add comments here about what is happening next? trunk/core/src/main/java/org/apache/oozie/store/StoreStatusFilter.java <https://reviews.apache.org/r/2948/#comment7905> Move the else to this line as per Java coding convention trunk/core/src/main/java/org/apache/oozie/store/StoreStatusFilter.java <https://reviews.apache.org/r/2948/#comment7903> Add comments about the block of code. Its a lot of code with no description of what to expect. trunk/core/src/main/java/org/apache/oozie/store/StoreStatusFilter.java <https://reviews.apache.org/r/2948/#comment7906> Can we use a string constant to avoid the duplicate use of "timeUnitStr". This will ensure that changes made to one place will propagate to the rest of the code. trunk/core/src/main/java/org/apache/oozie/store/StoreStatusFilter.java <https://reviews.apache.org/r/2948/#comment7904> Minor comment, use ++i. Pre-increment is usually faster. trunk/core/src/main/java/org/apache/oozie/store/StoreStatusFilter.java <https://reviews.apache.org/r/2948/#comment7907> Move the else to this line as per Java coding convention trunk/core/src/main/java/org/apache/oozie/store/StoreStatusFilter.java <https://reviews.apache.org/r/2948/#comment7908> Move the else to the previous line as per Java coding convention trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobInfoGetJPAExecutor.java <https://reviews.apache.org/r/2948/#comment7909> What is the purpose of this test? Can you add details? trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobInfoGetJPAExecutor.java <https://reviews.apache.org/r/2948/#comment7910> Remove this line since you have initialized frequencyList in the declaration. - Santhosh On 2011-11-28 19:56:10, Kiran Nagasubramanian wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2948/ > ----------------------------------------------------------- > > (Updated 2011-11-28 19:56:10) > > > Review request for oozie, Mohammad Islam and Angelo K. Huang. > > > Summary > ------- > > While specifying frequency value as part of -filter command option, it should > be possible to specify the timeunit also. > > > This addresses bug OOZIE-621. > https://issues.apache.org/jira/browse/OOZIE-621 > > > Diffs > ----- > > trunk/client/src/main/java/org/apache/oozie/client/OozieClient.java 1205116 > trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java 1205116 > trunk/core/src/main/java/org/apache/oozie/store/StoreStatusFilter.java > 1205116 > > trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobInfoGetJPAExecutor.java > 1205116 > > Diff: https://reviews.apache.org/r/2948/diff > > > Testing > ------- > > > Thanks, > > Kiran > >
