You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Satish Saley <sa...@gmail.com> on 2017/02/01 07:20:18 UTC
Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/
-----------------------------------------------------------
Review request for oozie.
Bugs: OOZIE-2630
https://issues.apache.org/jira/browse/OOZIE-2630
Repository: oozie-git
Description
-------
[OOZIE-2630] Amend patch for OOZIE-2630
Diffs
-----
core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
Diff: https://reviews.apache.org/r/56158/diff/
Testing
-------
Tested locally
Thanks,
Satish Saley
Re: Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
Posted by Satish Saley <sa...@gmail.com>.
> On Feb. 1, 2017, 1:02 a.m., Purshotam Shah wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java, line 410
> > <https://reviews.apache.org/r/56158/diff/3/?file=1621110#file1621110line410>
> >
> > End of functions calculation should not include initial instance calculation.
> > It's purely on nominal time. Endof functions at dataset frequency and at datasets instance should return the same value.
I am using dataset initial instance to get the hour:minute of it. Otherwise we might end up with the extra instances if nominal time is not in phase with the dataset time.
Consider a dataset with initial instance -
<dataset name='test' frequency='30' initial-instance='2009-08-01T00:06Z'
timezone='UTC' freq_timeunit='MINUTE' end_of_duration='NONE'>
...
<start-instance>${coord:endOfDays(0)}</start-instance>
<end-instance>${coord:current(0)}</end-instance>
And nominal time of 2009-08-20T18:00Z. The instances should start anytime after (endOfDays(0)), 2009-08-20T00:00Z. But without above adjustment, it calculates one instance from previous day 2009-08-19.
To solve this problem
1. Either, we have to ask users to make their timings in phase to avoid extra instance. In this case, we don't need amend patch.
2. Or, we can look at dataset initial instance hour:minute, and adjust the time.
- Satish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/#review163796
-----------------------------------------------------------
On Feb. 1, 2017, 12:44 a.m., Satish Saley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56158/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2017, 12:44 a.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2630
> https://issues.apache.org/jira/browse/OOZIE-2630
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> [OOZIE-2630] Amend patch for OOZIE-2630
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
> core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
> core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
> core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
> core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
> core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
>
> Diff: https://reviews.apache.org/r/56158/diff/
>
>
> Testing
> -------
>
> Tested locally
>
>
> Thanks,
>
> Satish Saley
>
>
Re: Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
Posted by Purshotam Shah <pu...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/#review163796
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java (line 384)
<https://reviews.apache.org/r/56158/#comment235302>
End of functions calculation should not include initial instance calculation.
It's purely on nominal time. Endof functions at dataset frequency and at datasets instance should return the same value.
- Purshotam Shah
On Feb. 1, 2017, 8:44 a.m., Satish Saley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56158/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2017, 8:44 a.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2630
> https://issues.apache.org/jira/browse/OOZIE-2630
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> [OOZIE-2630] Amend patch for OOZIE-2630
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
> core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
> core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
> core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
> core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
> core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
>
> Diff: https://reviews.apache.org/r/56158/diff/
>
>
> Testing
> -------
>
> Tested locally
>
>
> Thanks,
>
> Satish Saley
>
>
Re: Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
Posted by Satish Saley <sa...@gmail.com>.
> On Feb. 2, 2017, 4:33 p.m., Andr�s Piros wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java, lines 406-429
> > <https://reviews.apache.org/r/56158/diff/3/?file=1621110#file1621110line406>
> >
> > This `static` method I'd extract to a - maybe nested - class called `StartCalendarFinder` and use constructor parameters `Date nominalTime, Date initialTime, TimeZone timeZone, int startIndex, TimeUnit timeUnit`, and one sole `public Calendar findStartCalendar()` method.
Could you please mention any specific reasons for doing it that way? What advantages will it bring?
- Satish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/#review164056
-----------------------------------------------------------
On Feb. 5, 2017, 1 a.m., Satish Saley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56158/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2017, 1 a.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2630
> https://issues.apache.org/jira/browse/OOZIE-2630
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> [OOZIE-2630] Amend patch for OOZIE-2630
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
> core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
> core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
> core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
> core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
> core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
> core/src/test/resources/out-of-phase-coordinator.xml PRE-CREATION
> docs/src/site/twiki/CoordinatorFunctionalSpec.twiki 4b21ad9
>
> Diff: https://reviews.apache.org/r/56158/diff/
>
>
> Testing
> -------
>
> Tested locally
>
>
> Thanks,
>
> Satish Saley
>
>
Re: Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
Posted by Satish Saley <sa...@gmail.com>.
> On Feb. 2, 2017, 4:33 p.m., Andr�s Piros wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java, lines 406-429
> > <https://reviews.apache.org/r/56158/diff/3/?file=1621110#file1621110line406>
> >
> > This `static` method I'd extract to a - maybe nested - class called `StartCalendarFinder` and use constructor parameters `Date nominalTime, Date initialTime, TimeZone timeZone, int startIndex, TimeUnit timeUnit`, and one sole `public Calendar findStartCalendar()` method.
>
> Satish Saley wrote:
> Could you please mention any specific reasons for doing it that way? What advantages will it bring?
>
> Andr�s Piros wrote:
> Thanks for asking!
>
> The approach I suggested is better for code readability (Single Responsibility Principle - `CoordCommandUtils` does way too much also in the moment) and testability.
>
> I recommend reading Misko Hevery's testability blog, here is [the article](http://misko.hevery.com/code-reviewers-guide/flaw-brittle-global-state-singletons/) why we should use as little `static` methods as possible
I went through the article. It looks good. But I have +1 on current patch from Rohini. Let me check if she has cycles to review this change.
- Satish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/#review164056
-----------------------------------------------------------
On Feb. 5, 2017, 1 a.m., Satish Saley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56158/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2017, 1 a.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2630
> https://issues.apache.org/jira/browse/OOZIE-2630
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> [OOZIE-2630] Amend patch for OOZIE-2630
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
> core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
> core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
> core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
> core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
> core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
> core/src/test/resources/out-of-phase-coordinator.xml PRE-CREATION
> docs/src/site/twiki/CoordinatorFunctionalSpec.twiki 4b21ad9
>
> Diff: https://reviews.apache.org/r/56158/diff/
>
>
> Testing
> -------
>
> Tested locally
>
>
> Thanks,
>
> Satish Saley
>
>
Re: Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
Posted by András Piros <an...@cloudera.com>.
> On Feb. 3, 2017, 12:33 a.m., Andr�s Piros wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java, lines 406-429
> > <https://reviews.apache.org/r/56158/diff/3/?file=1621110#file1621110line406>
> >
> > This `static` method I'd extract to a - maybe nested - class called `StartCalendarFinder` and use constructor parameters `Date nominalTime, Date initialTime, TimeZone timeZone, int startIndex, TimeUnit timeUnit`, and one sole `public Calendar findStartCalendar()` method.
>
> Satish Saley wrote:
> Could you please mention any specific reasons for doing it that way? What advantages will it bring?
Thanks for asking!
The approach I suggested is better for code readability (Single Responsibility Principle - `CoordCommandUtils` does way too much also in the moment) and testability.
I recommend reading Misko Hevery's testability blog, here is [the article](http://misko.hevery.com/code-reviewers-guide/flaw-brittle-global-state-singletons/) why we should use as little `static` methods as possible
- Andr�s
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/#review164056
-----------------------------------------------------------
On Feb. 5, 2017, 9 a.m., Satish Saley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56158/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2017, 9 a.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2630
> https://issues.apache.org/jira/browse/OOZIE-2630
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> [OOZIE-2630] Amend patch for OOZIE-2630
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
> core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
> core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
> core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
> core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
> core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
> core/src/test/resources/out-of-phase-coordinator.xml PRE-CREATION
> docs/src/site/twiki/CoordinatorFunctionalSpec.twiki 4b21ad9
>
> Diff: https://reviews.apache.org/r/56158/diff/
>
>
> Testing
> -------
>
> Tested locally
>
>
> Thanks,
>
> Satish Saley
>
>
Re: Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/#review164056
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java (lines 380 - 403)
<https://reviews.apache.org/r/56158/#comment235603>
This `static` method I'd extract to a - maybe nested - class called `StartCalendarFinder` and use constructor parameters `Date nominalTime, Date initialTime, TimeZone timeZone, int startIndex, TimeUnit timeUnit`, and one sole `public Calendar findStartCalendar()` method.
- Andr�s Piros
On Feb. 1, 2017, 8:44 a.m., Satish Saley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56158/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2017, 8:44 a.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2630
> https://issues.apache.org/jira/browse/OOZIE-2630
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> [OOZIE-2630] Amend patch for OOZIE-2630
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
> core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
> core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
> core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
> core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
> core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
>
> Diff: https://reviews.apache.org/r/56158/diff/
>
>
> Testing
> -------
>
> Tested locally
>
>
> Thanks,
>
> Satish Saley
>
>
Re: Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/#review164389
-----------------------------------------------------------
Ship it!
I like it :)
- Andr�s Piros
On Feb. 6, 2017, 6:53 p.m., Satish Saley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56158/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2017, 6:53 p.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2630
> https://issues.apache.org/jira/browse/OOZIE-2630
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> [OOZIE-2630] Amend patch for OOZIE-2630
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
> core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
> core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
> core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
> core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
> core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
> core/src/test/resources/out-of-phase-coordinator.xml PRE-CREATION
> docs/src/site/twiki/CoordinatorFunctionalSpec.twiki 4b21ad9
>
> Diff: https://reviews.apache.org/r/56158/diff/
>
>
> Testing
> -------
>
> Tested locally
>
>
> Thanks,
>
> Satish Saley
>
>
Re: Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
Posted by Satish Saley <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/
-----------------------------------------------------------
(Updated Feb. 6, 2017, 10:53 a.m.)
Review request for oozie.
Changes
-------
- Creating a static class to calculate start instance
Bugs: OOZIE-2630
https://issues.apache.org/jira/browse/OOZIE-2630
Repository: oozie-git
Description
-------
[OOZIE-2630] Amend patch for OOZIE-2630
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
core/src/test/resources/out-of-phase-coordinator.xml PRE-CREATION
docs/src/site/twiki/CoordinatorFunctionalSpec.twiki 4b21ad9
Diff: https://reviews.apache.org/r/56158/diff/
Testing
-------
Tested locally
Thanks,
Satish Saley
Re: Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
Posted by Satish Saley <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/
-----------------------------------------------------------
(Updated Feb. 5, 2017, 1 a.m.)
Review request for oozie.
Bugs: OOZIE-2630
https://issues.apache.org/jira/browse/OOZIE-2630
Repository: oozie-git
Description
-------
[OOZIE-2630] Amend patch for OOZIE-2630
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
core/src/test/resources/out-of-phase-coordinator.xml PRE-CREATION
docs/src/site/twiki/CoordinatorFunctionalSpec.twiki 4b21ad9
Diff: https://reviews.apache.org/r/56158/diff/
Testing
-------
Tested locally
Thanks,
Satish Saley
Re: Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
Posted by Satish Saley <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/
-----------------------------------------------------------
(Updated Feb. 1, 2017, 12:44 a.m.)
Review request for oozie.
Bugs: OOZIE-2630
https://issues.apache.org/jira/browse/OOZIE-2630
Repository: oozie-git
Description
-------
[OOZIE-2630] Amend patch for OOZIE-2630
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
Diff: https://reviews.apache.org/r/56158/diff/
Testing
-------
Tested locally
Thanks,
Satish Saley
Re: Review Request 56158: [OOZIE-2630] Amend patch for OOZIE-2630
Posted by Satish Saley <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56158/
-----------------------------------------------------------
(Updated Feb. 1, 2017, 12:12 a.m.)
Review request for oozie.
Bugs: OOZIE-2630
https://issues.apache.org/jira/browse/OOZIE-2630
Repository: oozie-git
Description
-------
[OOZIE-2630] Amend patch for OOZIE-2630
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java 3a7a930
core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java bc18f4d
core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java 22d1f61
core/src/test/resources/coord-dataset-endOfDays.xml 6aa3f00
core/src/test/resources/coord-dataset-endOfMonths.xml 0ea5cac
core/src/test/resources/coord-dataset-endOfWeeks.xml 7879bf4
Diff: https://reviews.apache.org/r/56158/diff/
Testing
-------
Tested locally
Thanks,
Satish Saley