[
https://issues.apache.org/jira/browse/OOZIE-621?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13159036#comment-13159036
]
[email protected] commented on OOZIE-621:
-----------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2948/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-11-28 19:56:10)
bq.
bq.
bq. Review request for oozie, Mohammad Islam and Angelo K. Huang.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. While specifying frequency value as part of -filter command option, it
should be possible to specify the timeunit also.
bq.
bq.
bq. This addresses bug OOZIE-621.
bq. https://issues.apache.org/jira/browse/OOZIE-621
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. trunk/client/src/main/java/org/apache/oozie/client/OozieClient.java
1205116
bq. trunk/core/src/main/java/org/apache/oozie/CoordinatorEngine.java 1205116
bq. trunk/core/src/main/java/org/apache/oozie/store/StoreStatusFilter.java
1205116
bq.
trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobInfoGetJPAExecutor.java
1205116
bq.
bq. Diff: https://reviews.apache.org/r/2948/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Kiran
bq.
bq.
> Option to add timeunit for frequency for coordinator job log filtering
> ----------------------------------------------------------------------
>
> Key: OOZIE-621
> URL: https://issues.apache.org/jira/browse/OOZIE-621
> Project: Oozie
> Issue Type: Improvement
> Reporter: Kiran Nagasubramanian
>
> While specifying frequency value as part of -filter command option, it should
> be possible to specify the timeunit also. The timeunit can take one of the
> following four values: months, days, hours or minutes, all being case
> sensitive. Find the example below:
> oozie jobs -jobtype coord -filter frequency=60;unit=minutes
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
