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