You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Kiran Nagasubramanian <nk...@gmail.com> on 2011/11/28 20:56:10 UTC
Review Request: Option to add timeunit for frequency for coordinator job log
filtering
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2948/
-----------------------------------------------------------
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
Re: Review Request: Option to add timeunit for frequency for coordinator job
log filtering
Posted by Santhosh Srinivasan <sm...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: Option to add timeunit for frequency for coordinator
jobs filtering
Posted by Mohammad Islam <mi...@yahoo.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2948/#review3830
-----------------------------------------------------------
Ship it!
+1
Committed.
Please close review request and the JIRA as fixed.
- Mohammad
On 2011-12-09 19:54:10, Kiran Nagasubramanian wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2948/
> -----------------------------------------------------------
>
> (Updated 2011-12-09 19:54: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/TestCoordinatorEngine.java 1205116
> trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobInfoGetJPAExecutor.java 1205116
> trunk/release-log.txt 1205116
>
> Diff: https://reviews.apache.org/r/2948/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kiran
>
>
Re: Review Request: Option to add timeunit for frequency for coordinator
jobs filtering
Posted by Kiran Nagasubramanian <nk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2948/
-----------------------------------------------------------
(Updated 2011-12-09 19:54:10.193047)
Review request for oozie, Mohammad Islam and Angelo K. Huang.
Summary (updated)
-------
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/TestCoordinatorEngine.java 1205116
trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobInfoGetJPAExecutor.java 1205116
trunk/release-log.txt 1205116
Diff: https://reviews.apache.org/r/2948/diff
Testing
-------
Thanks,
Kiran
Re: Review Request: Option to add timeunit for frequency for coordinator job
log filtering
Posted by Santhosh Srinivasan <sm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2948/#review3632
-----------------------------------------------------------
Ship it!
+1
- Santhosh
On 2011-12-05 17:41:05, Kiran Nagasubramanian wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2948/
> -----------------------------------------------------------
>
> (Updated 2011-12-05 17:41:05)
>
>
> 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/TestCoordinatorEngine.java 1205116
> trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobInfoGetJPAExecutor.java 1205116
> trunk/release-log.txt 1205116
>
> Diff: https://reviews.apache.org/r/2948/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Kiran
>
>
Re: Review Request: Option to add timeunit for frequency for coordinator job
log filtering
Posted by Kiran Nagasubramanian <nk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2948/
-----------------------------------------------------------
(Updated 2011-12-05 17:41:05.260335)
Review request for oozie, Mohammad Islam and Angelo K. Huang.
Changes
-------
Have incorporated all the four comments.
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 (updated)
-----
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/TestCoordinatorEngine.java 1205116
trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobInfoGetJPAExecutor.java 1205116
trunk/release-log.txt 1205116
Diff: https://reviews.apache.org/r/2948/diff
Testing
-------
Thanks,
Kiran
Re: Review Request: Option to add timeunit for frequency for coordinator job
log filtering
Posted by Kiran Nagasubramanian <nk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2948/
-----------------------------------------------------------
(Updated 2011-12-01 20:55:19.781934)
Review request for oozie, Mohammad Islam and Angelo K. Huang.
Changes
-------
Have incorporated the changes.
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 (updated)
-----
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/TestCoordinatorEngine.java 1205116
trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobInfoGetJPAExecutor.java 1205116
trunk/release-log.txt 1205116
Diff: https://reviews.apache.org/r/2948/diff
Testing
-------
Thanks,
Kiran