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