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