> 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 > >
