----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57680/#review169109 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java Lines 82 (patched) <https://reviews.apache.org/r/57680/#comment241428> `sharelibName` would be a better name core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java Lines 82-88 (original), 83-89 (patched) <https://reviews.apache.org/r/57680/#comment241427> tabs vs spaces core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java Line 83 (original), 84 (patched) <https://reviews.apache.org/r/57680/#comment241429> What about extracting `SparkConfigurationService` to a local variable? See Law of Demeter core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Lines 39-47 (original), 39-47 (patched) <https://reviews.apache.org/r/57680/#comment241434> Honestly, either more descriptive constant names, or javadoc, or a neatly named builder would be of great value here. core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Line 40 (original), 40 (patched) <https://reviews.apache.org/r/57680/#comment241432> Why not call that `CONFIGURATIONS_SUFFIX` instead? core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Line 43 (original), 43 (patched) <https://reviews.apache.org/r/57680/#comment241433> Why not call that rather `BLACKLIST_SUFFIX`? core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Line 48 (original), 48 (patched) <https://reviews.apache.org/r/57680/#comment241430> Every time I see something like `Map<String, Map<String, Something>>` reminds me always to use Guava's `Multimap<String, Something>`: https://github.com/google/guava/wiki/NewCollectionTypesExplained#multimap core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Lines 54 (patched) <https://reviews.apache.org/r/57680/#comment241439> Are you sure `^` is not needed as a prefix, and `$` is needed at the end? It would be best to have some unit tests that show what regexps are accepted and which ones not. core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Lines 55 (patched) <https://reviews.apache.org/r/57680/#comment241443> `sparkConfigName` core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Lines 76 (patched) <https://reviews.apache.org/r/57680/#comment241440> What about using `LinkedHashSet` which preserves addition order? core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Lines 77 (patched) <https://reviews.apache.org/r/57680/#comment241444> `blacklistedPropertyName` core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Lines 81-102 (original), 89-114 (patched) <https://reviews.apache.org/r/57680/#comment241445> This functionality should really be extracted to a separate `SparkConfigLoader` class - `SparkConfigurationService` does way too much ATM. core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Lines 119-123 (original), 131-135 (patched) <https://reviews.apache.org/r/57680/#comment241453> Should also be extracted to a separate class. core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Lines 145-153 (original), 160-168 (patched) <https://reviews.apache.org/r/57680/#comment241454> tabs vs spaces core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java Line 50 (original), 50 (patched) <https://reviews.apache.org/r/57680/#comment241456> This test case is really undreadable. Can you please split that up to multiple test cases and add new ones that cover your new regex and different kinds of Spark configuration mixes that cover the use cases we'd like to address: Spark 1.6 standalone, Spark 2.1 standalone, Spark 1.6 w/ Spark 2.0 w/ Spark 2.1, etc. core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java Line 63 (original), 63 (patched) <https://reviews.apache.org/r/57680/#comment241455> Maybe extract `"spark"` to a constant? core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java Lines 160 (patched) <https://reviews.apache.org/r/57680/#comment241458> From the test method name alone I cannot conclude what is the test case performing and what are the expected outcomes / end states. Please rename it. core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java Lines 161-163 (patched) <https://reviews.apache.org/r/57680/#comment241459> Wow, I cannot really understand who is who, the sheer amount of one-letter constants is amazing. core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java Lines 173-175 (patched) <https://reviews.apache.org/r/57680/#comment241461> Maybe use the same Builder here, or `String.format()` w/ the same constant would be less copy-and-paste-error prone. - András Piros On March 16, 2017, 9:32 a.m., Peter Cseh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57680/ > ----------------------------------------------------------- > > (Updated March 16, 2017, 9:32 a.m.) > > > Review request for oozie. > > > Bugs: OOZIE-2812 > https://issues.apache.org/jira/browse/OOZIE-2812 > > > Repository: oozie-git > > > Description > ------- > > OOZIE-2812 > SparkConfigurationService should support loading configurations from multiple > Spark versions > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java > 1a3197abb52f80b545dcc8634a8cac7a281a9eac > core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java > b15cce0d6813375778abf1d2bbe09e83734d19f0 > > core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java > 0e00a457890d1f4b8e86c19043cd597928cdd9bb > > > Diff: https://reviews.apache.org/r/57680/diff/1/ > > > Testing > ------- > > > Thanks, > > Peter Cseh > >
