You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by András Piros via Review Board <no...@reviews.apache.org> on 2017/10/12 15:33:31 UTC
Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/
-----------------------------------------------------------
Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
Repository: oozie-git
Description
-------
OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
Diffs
-----
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java PRE-CREATION
core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationFilter.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
Diff: https://reviews.apache.org/r/62936/diff/1/
Testing
-------
JUnit tests:
* `TestJavaActionExecutor`
* `TestLauncherConfigurationFilter`
Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
* only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
* `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
* only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
Thanks,
András Piros
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Peter Cseh via Review Board <no...@reviews.apache.org>.
> On Oct. 12, 2017, 11:36 p.m., Robert Kanter wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
> > Lines 1197 (patched)
> > <https://reviews.apache.org/r/62936/diff/1/?file=1853365#file1853365line1233>
> >
> > This is fine to leave for a different JIRA, but we should consider dropping the "oozie:launcher:" part of the name. The job type is now OOZIE, so it's probably redundent and we can shorten the names.
OOZIE-2893
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review187893
-----------------------------------------------------------
On Oct. 12, 2017, 3:33 p.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2017, 3:33 p.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java PRE-CREATION
> core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationFilter.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/1/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
> On Oct. 12, 2017, 11:36 p.m., Robert Kanter wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
> > Line 17 (original), 17 (patched)
> > <https://reviews.apache.org/r/62936/diff/1/?file=1853365#file1853365line17>
> >
> > Don't forget to add documentation. I'm not sure where is the best place though. I guess somewhere on the workflow page?
Talked to Geza regarding this one:
* for the users, it should be transparent which parameters override which other ones
* ATM we don't have a place within the docs, and we don't much have docs about launcher or mapper stuff
* we will in [OOZIE-2600](https://issues.apache.org/jira/browse/OOZIE-2600) what I've updated not to forget this override / prepend story
> On Oct. 12, 2017, 11:36 p.m., Robert Kanter wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
> > Lines 1154 (patched)
> > <https://reviews.apache.org/r/62936/diff/1/?file=1853365#file1853365line1190>
> >
> > Shouldn't we check the workflow.xml first? Same with the others.
Since `oozie.launcher.override.*` will have overridden `oozie.launcher.*` by the time of this method to run, it works like expected ATM. Same for `oozie.launcher.prepend.*`.
> On Oct. 12, 2017, 11:36 p.m., Robert Kanter wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
> > Lines 29 (patched)
> > <https://reviews.apache.org/r/62936/diff/1/?file=1853366#file1853366line29>
> >
> > Should we make this all static? It doesn't really have any state and we're always calling it like:
> > ````
> > new LauncherConfigurationFilter(actionDefaultConf).filter(launcherConf);
> > ````
> >
> > We could avoid creating the object by doing:
> > ````
> > LauncherConfigurationFilter.filter(actionDefaultConf, launcherConf);
> > ````
I am against yet another method parameter (IMO the less the better), and in favor of having the `sourceConfiguration` as field.
- András
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review187893
-----------------------------------------------------------
On Oct. 12, 2017, 3:33 p.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2017, 3:33 p.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java PRE-CREATION
> core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationFilter.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/1/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Robert Kanter via Review Board <no...@reviews.apache.org>.
> On Oct. 12, 2017, 11:36 p.m., Robert Kanter wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
> > Lines 1197 (patched)
> > <https://reviews.apache.org/r/62936/diff/1/?file=1853365#file1853365line1233>
> >
> > This is fine to leave for a different JIRA, but we should consider dropping the "oozie:launcher:" part of the name. The job type is now OOZIE, so it's probably redundent and we can shorten the names.
>
> Peter Cseh wrote:
> OOZIE-2893
Oh I forgot about that Haha. At least I'm being consistent with the name :)
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review187893
-----------------------------------------------------------
On Oct. 12, 2017, 3:33 p.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2017, 3:33 p.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java PRE-CREATION
> core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationFilter.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/1/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Robert Kanter via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review187893
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Line 17 (original), 17 (patched)
<https://reviews.apache.org/r/62936/#comment264948>
Don't forget to add documentation. I'm not sure where is the best place though. I guess somewhere on the workflow page?
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1154 (patched)
<https://reviews.apache.org/r/62936/#comment264936>
Shouldn't we check the workflow.xml first? Same with the others.
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1197 (patched)
<https://reviews.apache.org/r/62936/#comment264938>
This is fine to leave for a different JIRA, but we should consider dropping the "oozie:launcher:" part of the name. The job type is now OOZIE, so it's probably redundent and we can shorten the names.
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1211 (patched)
<https://reviews.apache.org/r/62936/#comment264942>
Should we log something here? A stack trace is unnecessary, but maybe something like "Invalid integer [X] specified for max attempts, using default [Y]".
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1217-1221 (patched)
<https://reviews.apache.org/r/62936/#comment264944>
The log message might be misleading if you had a negative value. It would say that we're setting it, but then the if statement would make it not.
Also, should we log something in the case of a negative value?
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 29 (patched)
<https://reviews.apache.org/r/62936/#comment264946>
Should we make this all static? It doesn't really have any state and we're always calling it like:
````
new LauncherConfigurationFilter(actionDefaultConf).filter(launcherConf);
````
We could avoid creating the object by doing:
````
LauncherConfigurationFilter.filter(actionDefaultConf, launcherConf);
````
- Robert Kanter
On Oct. 12, 2017, 3:33 p.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2017, 3:33 p.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java PRE-CREATION
> core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationFilter.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/1/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
> On Oct. 17, 2017, 11:24 a.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
> > Lines 256 (patched)
> > <https://reviews.apache.org/r/62936/diff/2/?file=1857307#file1857307line256>
> >
> > So if we define oozie.launcher.something = 1, we'll have "oozie.launcher.something" and "something" as keys, with values = 1, correct?
> >
> > Is this what we want?
Yes, this is correct, and this is the original behavior - this is what we want here. An example is created, and there is `TestLauncherConfigurationInjector#testOverrideSwitchedOffSourceCopiedToTargetWithTwoDifferentKeys()` that cover exactly this scenario.
- András
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review188290
-----------------------------------------------------------
On Oct. 16, 2017, 4:19 p.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2017, 4:19 p.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/2/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review188290
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 32-61 (patched)
<https://reviews.apache.org/r/62936/#comment265297>
This is a good way to describe how the mechanism works in general.
But it's still very difficult to figure how things are happening under the hood.
Eg. how the property "mapreduce.map.cpu.vcores" eventually becomes "oozie.launcher.vcores". I think I understand it now, but it's rather complicated.
Let's discuss this later.
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 41 (patched)
<https://reviews.apache.org/r/62936/#comment265294>
with instead of "w/"
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 47 (patched)
<https://reviews.apache.org/r/62936/#comment265295>
"with" instead of "w/"
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 57 (patched)
<https://reviews.apache.org/r/62936/#comment265292>
Could you rephrase this? It's weird to read "as defined" then "are defined".
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 95-96 (patched)
<https://reviews.apache.org/r/62936/#comment265298>
We really need simple examples as well. That's generally true for other local methods.
Choose an arbitrary property and describe what the contents of the multimap become after this method runs.
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 129 (patched)
<https://reviews.apache.org/r/62936/#comment265288>
Possible typo: "overridden", "prepended"
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 256 (patched)
<https://reviews.apache.org/r/62936/#comment265290>
So if we define oozie.launcher.something = 1, we'll have "oozie.launcher.something" and "something" as keys, with values = 1, correct?
Is this what we want?
- Peter Bacsko
On okt. 16, 2017, 4:19 du, András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated okt. 16, 2017, 4:19 du)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/2/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
> On okt. 17, 2017, 3:19 du, András Piros wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
> > Lines 95-96 (patched)
> > <https://reviews.apache.org/r/62936/diff/2/?file=1857307#file1857307line95>
> >
> > I'm really rather in favor of making code more readable by separating concerns, using descriptive names, and having reasonable unit tests. In fact unit tests help me a lot usually when reading code.
> >
> > Putting examples (or even, Javadoc) on `private` methods that may change at any gives point of time seems to be very counterproductive to me. If you like, I can extract this (or any other) functionality to other classes and make separate unit tests for these.
Perhaps I wasn't quite clear.
The comments on individual methods are good but it didn't help me understand the mechanics of the entire override/prepend concept.
Some examples:
- I didn't understand what the multimaps are for
- I didn't understand why we need a "linkedHashMultiMap"
- The method fillConfigPropertiesByRegex() is not obvious at first look
- fillOverridesOrPrepends() seems to perform an even bigger magic
After reading the code thoroughly, I think I get it. But it took multiple attempts.
Proper naming and having good unit tests are necessary, but at times not suffucient and I believe this is one of them.
If, however, we describe what the multimap stores with an example and longer explanation, things might become immediately clear. At least the enlightenment happens a lot quicker:)
It does not have to be JavaDoc though. In fact, perhaps it's better not to have these kind of descriptions as JavaDoc. For example, look at these comment what I attached to LiteWorkfFlowValidator: https://github.com/apache/oozie/blob/8e9b9042b3270dc5ff975c44a5c977fcc41250e4/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java#L149-L176
https://github.com/apache/oozie/blob/8e9b9042b3270dc5ff975c44a5c977fcc41250e4/core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowValidator.java#L191-L208
You can't properly explain this piece of code with good naming convention and tests.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review188316
-----------------------------------------------------------
On okt. 17, 2017, 3:25 du, András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated okt. 17, 2017, 3:25 du)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/3/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review188316
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 32-61 (patched)
<https://reviews.apache.org/r/62936/#comment265306>
Writing examples to all items in the list to make the point more clear.
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 95-96 (patched)
<https://reviews.apache.org/r/62936/#comment265308>
I'm really rather in favor of making code more readable by separating concerns, using descriptive names, and having reasonable unit tests. In fact unit tests help me a lot usually when reading code.
Putting examples (or even, Javadoc) on `private` methods that may change at any gives point of time seems to be very counterproductive to me. If you like, I can extract this (or any other) functionality to other classes and make separate unit tests for these.
- András Piros
On Oct. 16, 2017, 4:19 p.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2017, 4:19 p.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/2/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
> On Oct. 19, 2017, 2:10 p.m., Peter Bacsko wrote:
> > core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java
> > Lines 26 (patched)
> > <https://reviews.apache.org/r/62936/diff/4/?file=1863481#file1863481line26>
> >
> > There are a lot of hard-coded stuff in this class, especially test data.
> >
> > I know that we don't import the constant fields which define properties, but I think at least the test-specific stuff could be extracted to final fields, especially if it's repeated.
Moved to `SourceConfigurationFactory` nested class for better readability and separation of concerns.
> On Oct. 19, 2017, 2:10 p.m., Peter Bacsko wrote:
> > core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java
> > Lines 130 (patched)
> > <https://reviews.apache.org/r/62936/diff/4/?file=1863481#file1863481line130>
> >
> > Are you intentionally using "true" here, and "false" above?
Yes. Source `Configuration` needs always to be created with defaults, target launcher `Configuration` always without defaults. Extracted to better-named methods, `newConfigurationWithoutDefaults()` and `newConfigurationWithDefaults()`.
> On Oct. 19, 2017, 2:10 p.m., Peter Bacsko wrote:
> > core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java
> > Lines 132 (patched)
> > <https://reviews.apache.org/r/62936/diff/4/?file=1863481#file1863481line132>
> >
> > Instead of Integer.toString(4), wouldn't it be easier just to simply type "4"? Does this have an advantage?
Extracted every occurrence of `Integer#toString()` to `String` values.
- András
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review188678
-----------------------------------------------------------
On Oct. 19, 2017, 10:04 a.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2017, 10:04 a.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/4/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review188678
-----------------------------------------------------------
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java
Lines 26 (patched)
<https://reviews.apache.org/r/62936/#comment265696>
There are a lot of hard-coded stuff in this class, especially test data.
I know that we don't import the constant fields which define properties, but I think at least the test-specific stuff could be extracted to final fields, especially if it's repeated.
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java
Lines 130 (patched)
<https://reviews.apache.org/r/62936/#comment265694>
Are you intentionally using "true" here, and "false" above?
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java
Lines 132 (patched)
<https://reviews.apache.org/r/62936/#comment265695>
Instead of Integer.toString(4), wouldn't it be easier just to simply type "4"? Does this have an advantage?
- Peter Bacsko
On okt. 19, 2017, 10:04 de, András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated okt. 19, 2017, 10:04 de)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/4/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Peter Cseh via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review189412
-----------------------------------------------------------
Ship it!
Ship It!
- Peter Cseh
On Oct. 27, 2017, 10:01 a.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2017, 10:01 a.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/6/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review189417
-----------------------------------------------------------
Ship it!
- Peter Bacsko
On okt. 27, 2017, 10:01 de, András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated okt. 27, 2017, 10:01 de)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/6/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/
-----------------------------------------------------------
(Updated Oct. 27, 2017, 10:01 a.m.)
Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
Changes
-------
Last (?) review round.
Repository: oozie-git
Description
-------
OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
Diff: https://reviews.apache.org/r/62936/diff/6/
Changes: https://reviews.apache.org/r/62936/diff/5-6/
Testing
-------
JUnit tests:
* `TestJavaActionExecutor`
* `TestLauncherConfigurationFilter`
Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
* only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
* `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
* only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
Thanks,
András Piros
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
> On Oct. 27, 2017, 9:04 a.m., Peter Cseh wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
> > Lines 316 (patched)
> > <https://reviews.apache.org/r/62936/diff/4/?file=1863477#file1863477line316>
> >
> > Does this mean that we're not overwriting things if there was already a value given for it?
Based on [Rohini's](https://issues.apache.org/jira/browse/OOZIE-2896?focusedCommentId=16189031&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16189031) and [Robert's](https://issues.apache.org/jira/browse/OOZIE-2896?focusedCommentId=16190510&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16190510) comments, if there is an `oozie.launcher.*` property defined in either the `workflow.xml` or in the `oozie-[site,default].xml`, it has precendence always over the override value.
> On Oct. 27, 2017, 9:04 a.m., Peter Cseh wrote:
> > core/src/main/resources/oozie-default.xml
> > Lines 3120 (patched)
> > <https://reviews.apache.org/r/62936/diff/4/?file=1863479#file1863479line3120>
> >
> > I don't like that it's max.attempts in most cases and max-attempts here.
New value is `oozie.laucnher.default.max.attempts`.
- András
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review189406
-----------------------------------------------------------
On Oct. 26, 2017, 5:20 p.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2017, 5:20 p.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/5/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Peter Cseh via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review189406
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 316 (patched)
<https://reviews.apache.org/r/62936/#comment266502>
Does this mean that we're not overwriting things if there was already a value given for it?
core/src/main/resources/oozie-default.xml
Lines 3120 (patched)
<https://reviews.apache.org/r/62936/#comment266501>
I don't like that it's max.attempts in most cases and max-attempts here.
- Peter Cseh
On Oct. 26, 2017, 5:20 p.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2017, 5:20 p.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/5/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/
-----------------------------------------------------------
(Updated Oct. 26, 2017, 5:20 p.m.)
Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
Changes
-------
Another review comments round.
Repository: oozie-git
Description
-------
OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
Diff: https://reviews.apache.org/r/62936/diff/5/
Changes: https://reviews.apache.org/r/62936/diff/4-5/
Testing
-------
JUnit tests:
* `TestJavaActionExecutor`
* `TestLauncherConfigurationFilter`
Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
* only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
* `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
* only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
Thanks,
András Piros
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
> On Oct. 19, 2017, 3:03 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
> > Lines 133 (patched)
> > <https://reviews.apache.org/r/62936/diff/4/?file=1863476#file1863476line155>
> >
> > Any plans to add this to the XML schema? Or it's not that useful?
At the moment it's not that super useful, so decided not to add to the XML schema by now.
> On Oct. 19, 2017, 3:03 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
> > Lines 83-99 (patched)
> > <https://reviews.apache.org/r/62936/diff/4/?file=1863477#file1863477line83>
> >
> > Now that's what I'm talkin' about! :D
:D
> On Oct. 19, 2017, 3:03 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
> > Lines 224-241 (patched)
> > <https://reviews.apache.org/r/62936/diff/4/?file=1863477#file1863477line224>
> >
> > Good comment again.
> >
> > Perhaps it would be worthwile to point out that "oozie.launcher.override.XX" will be "oozie.launcher.XX" just to be absolutely clear.
Done.
- András
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review188680
-----------------------------------------------------------
On Oct. 19, 2017, 10:04 a.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2017, 10:04 a.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/4/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
> On Oct. 19, 2017, 3:03 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
> > Lines 133 (patched)
> > <https://reviews.apache.org/r/62936/diff/4/?file=1863476#file1863476line155>
> >
> > Any plans to add this to the XML schema? Or it's not that useful?
>
> András Piros wrote:
> At the moment it's not that super useful, so decided not to add to the XML schema by now.
It's not that super useful, won't extend XML schema atm.
- András
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review188680
-----------------------------------------------------------
On Oct. 19, 2017, 10:04 a.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2017, 10:04 a.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/4/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review188680
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 133 (patched)
<https://reviews.apache.org/r/62936/#comment265699>
Any plans to add this to the XML schema? Or it's not that useful?
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 83-99 (patched)
<https://reviews.apache.org/r/62936/#comment265697>
Now that's what I'm talkin' about! :D
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java
Lines 224-241 (patched)
<https://reviews.apache.org/r/62936/#comment265700>
Good comment again.
Perhaps it would be worthwile to point out that "oozie.launcher.override.XX" will be "oozie.launcher.XX" just to be absolutely clear.
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
Lines 2564 (patched)
<https://reviews.apache.org/r/62936/#comment265698>
Add assertion failure message
- Peter Bacsko
On okt. 19, 2017, 10:04 de, András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated okt. 19, 2017, 10:04 de)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
> core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/4/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/
-----------------------------------------------------------
(Updated Oct. 19, 2017, 10:04 a.m.)
Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
Changes
-------
Updating more review comments.
Repository: oozie-git
Description
-------
OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
Diff: https://reviews.apache.org/r/62936/diff/4/
Changes: https://reviews.apache.org/r/62936/diff/3-4/
Testing
-------
JUnit tests:
* `TestJavaActionExecutor`
* `TestLauncherConfigurationFilter`
Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
* only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
* `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
* only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
Thanks,
András Piros
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/
-----------------------------------------------------------
(Updated Oct. 17, 2017, 3:25 p.m.)
Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
Changes
-------
Addressing review comments.
Repository: oozie-git
Description
-------
OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
core/src/main/resources/oozie-default.xml 9ba8fd4a0d9dd5f0bd6a64af3724c2f910d86457
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
Diff: https://reviews.apache.org/r/62936/diff/3/
Changes: https://reviews.apache.org/r/62936/diff/2-3/
Testing
-------
JUnit tests:
* `TestJavaActionExecutor`
* `TestLauncherConfigurationFilter`
Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
* only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
* `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
* only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
Thanks,
András Piros
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/
-----------------------------------------------------------
(Updated Oct. 16, 2017, 4:19 p.m.)
Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
Changes
-------
Addressing review comments.
Repository: oozie-git
Description
-------
OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationInjector.java PRE-CREATION
core/src/main/java/org/apache/oozie/service/ConfigurationService.java a51933022c14ea4ce06a65cc3c123bed8a8f7f34
core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationInjector.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
Diff: https://reviews.apache.org/r/62936/diff/2/
Changes: https://reviews.apache.org/r/62936/diff/1-2/
Testing
-------
JUnit tests:
* `TestJavaActionExecutor`
* `TestLauncherConfigurationFilter`
Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
* only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
* `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
* only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
Thanks,
András Piros
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by András Piros via Review Board <no...@reviews.apache.org>.
> On Oct. 16, 2017, 12:51 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
> > Lines 29 (patched)
> > <https://reviews.apache.org/r/62936/diff/1/?file=1853366#file1853366line29>
> >
> > Should we come up with some better name?
> >
> > Like LauncherConfigurationProcessor or I don't know. Filter sound like we're removing stuff.
Renamed to `LauncherConfigurationInjector#inject()`.
> On Oct. 16, 2017, 12:51 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
> > Lines 61 (patched)
> > <https://reviews.apache.org/r/62936/diff/1/?file=1853366#file1853366line61>
> >
> > Can't this method be just simply static?
> >
> > I don't think we should always instantiate a class for the sake of a simple method invocation.
I am against yet another method parameter (IMO the less the better), and in favor of having the `sourceConfiguration` as field.
> On Oct. 16, 2017, 12:51 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
> > Lines 130-132 (patched)
> > <https://reviews.apache.org/r/62936/diff/1/?file=1853366#file1853366line130>
> >
> > What's happening here? An override then a prepend?
This part is refactored, but nevertheless, using hopefully better names and also method level Javadoc.
> On Oct. 16, 2017, 12:51 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
> > Lines 134-136 (patched)
> > <https://reviews.apache.org/r/62936/diff/1/?file=1853366#file1853366line134>
> >
> > Same here
This part is refactored, but nevertheless, using hopefully better names and also method level Javadoc.
- András
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review188120
-----------------------------------------------------------
On Oct. 12, 2017, 3:33 p.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2017, 3:33 p.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java PRE-CREATION
> core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationFilter.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/1/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review188120
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1247 (patched)
<https://reviews.apache.org/r/62936/#comment265168>
Nit: space before "launcherJobConf"
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 29 (patched)
<https://reviews.apache.org/r/62936/#comment265170>
Should we come up with some better name?
Like LauncherConfigurationProcessor or I don't know. Filter sound like we're removing stuff.
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 57 (patched)
<https://reviews.apache.org/r/62936/#comment265178>
This class needs to be documented massively.
I scrolled up and down at least a dozen times but I'm still confused about what's going on here.
Eg. what happens in case of conflicting properties?
What happens if we define something in a <launcher> tag which happens to collide with an MR2 property defined as oozie.launcher.mapreduce.whatever.
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 61 (patched)
<https://reviews.apache.org/r/62936/#comment265167>
Can't this method be just simply static?
I don't think we should always instantiate a class for the sake of a simple method invocation.
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 77 (patched)
<https://reviews.apache.org/r/62936/#comment265174>
Pls add JavaDoc about what this method does, because it's not immediately obvious what we fill here.
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 130-132 (patched)
<https://reviews.apache.org/r/62936/#comment265176>
What's happening here? An override then a prepend?
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 134-136 (patched)
<https://reviews.apache.org/r/62936/#comment265177>
Same here
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 151 (patched)
<https://reviews.apache.org/r/62936/#comment265173>
Could you pls add a short Javadoc which describes what this method does and why. Would be good to add an example.
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 163 (patched)
<https://reviews.apache.org/r/62936/#comment265175>
Pls document the purpose of this method.
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 170 (patched)
<https://reviews.apache.org/r/62936/#comment265171>
Missing parenthesis ")"
The comment seems to into a single line
- Peter Bacsko
On okt. 12, 2017, 3:33 du, András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated okt. 12, 2017, 3:33 du)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java PRE-CREATION
> core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationFilter.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/1/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>
Re: Review Request 62936: OOZIE-2896 Ensure compatibility for existing
LauncherMapper settings
Posted by Peter Cseh via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62936/#review187806
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1174-1175 (original), 1148-1183 (patched)
<https://reviews.apache.org/r/62936/#comment264842>
Why move this huge block? Things like this make backports harder.
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Line 1183 (original), 1259 (patched)
<https://reviews.apache.org/r/62936/#comment264843>
Shouldn't the ",CLA" part be kept here?
If it's just the log level, the appender should stay the same.
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 32-52 (patched)
<https://reviews.apache.org/r/62936/#comment264851>
Please use the configuration.getValByRegex() and use oozie.launcher.override.* and oozie.launcher.prepend.*
Also, we're calling the LauncherAM as "launcher" in the configfiles, not "am" we should use the same properties from LauncherAM like LauncherAM.OOZIE_LAUNCHER_VCORES_PROPERTY and use the prefixes for it.
You could do ALL the overrites first and all the prepends later.
This way when we add a new property, it will just work. (I may come handy when resourcetypes are released in Hadoop)
core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java
Lines 61 (patched)
<https://reviews.apache.org/r/62936/#comment264844>
this method does not filter. It injects/copies properties over.
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
Line 2183 (original), 2183 (patched)
<https://reviews.apache.org/r/62936/#comment264846>
What is the extra property appearing here? The converted attempt count?
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
Lines 2548-2563 (patched)
<https://reviews.apache.org/r/62936/#comment264847>
There are a whole lot of things set here without any assertion in the test down the road.
Either remove the unneccessary properties or assert them.
- Peter Cseh
On Oct. 12, 2017, 3:33 p.m., András Piros wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62936/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2017, 3:33 p.m.)
>
>
> Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2896 Ensure compatibility for existing LauncherMapper settings
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 76d0daa4f7528de6fb3c8993bae4dd28b6f7865b
> core/src/main/java/org/apache/oozie/action/hadoop/LauncherConfigurationFilter.java PRE-CREATION
> core/src/main/resources/oozie-default.xml 2389b993b7b1769521d6dd0c7386e99e7516210a
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java a7bd357f8938d148a150db43135a7a8fe7f0da4c
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherConfigurationFilter.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java ee1a32ac9b071009e1dbe78396520d778f58bddc
>
>
> Diff: https://reviews.apache.org/r/62936/diff/1/
>
>
> Testing
> -------
>
> JUnit tests:
>
> * `TestJavaActionExecutor`
> * `TestLauncherConfigurationFilter`
>
> Functional tests: submit and run `examples/apps/java-main/workflow.xml` in three modes:
>
> * only `oozie.launcher.` parameters are present: `oozie.launcher.` parameters are passed through
> * `oozie.launcher.` and override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are also present: `oozie.launcher.` parameters are passed through
> * only override (`yarn.`, `mapreduce.`, and `mapred.`) parameters are present: override parameters are passed through
>
>
> Thanks,
>
> András Piros
>
>