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