----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57680/#review174010 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/service/sparkconfiguration/SparkConfigReader.java Lines 18 (patched) <https://reviews.apache.org/r/57680/#comment247110> Q: Other Oozie components could also benefit from having a separate package? Package name ``org.apache.oozie.service.sparkconfiguration`` might be ``org.apache.oozie.service.configuration.spark`` ? core/src/main/java/org/apache/oozie/service/sparkconfiguration/SparkConfigReader.java Lines 50 (patched) <https://reviews.apache.org/r/57680/#comment247107> nit: You don't need this else part if you return like above core/src/main/java/org/apache/oozie/service/sparkconfiguration/SparkConfigReader.java Lines 63 (patched) <https://reviews.apache.org/r/57680/#comment247106> nit: applyBlackList sounds better to me. Reading method name filterBlackList suggested that you were filtering the blacklist itself. core/src/main/java/org/apache/oozie/service/sparkconfiguration/SparkConfigurationStorage.java Lines 86 (patched) <https://reviews.apache.org/r/57680/#comment247109> Is it really a good idea to continue the execution if we detect invalid configuration? core/src/main/java/org/apache/oozie/service/sparkconfiguration/SparkConfigurationStorage.java Lines 118 (patched) <https://reviews.apache.org/r/57680/#comment247103> nit: no else part need if you ``return config`` above, I would probablu do this check earlier and return. I would log "no Spark config exists for this sharelib." and the sharelib instead of having it as a comment. core/src/test/java/org/apache/oozie/service/sparkconfiguration/TestSparkConfigurationStorage.java Lines 54 (patched) <https://reviews.apache.org/r/57680/#comment247104> nit: assertEquals would be better - Attila Sasvari On March 22, 2017, 10:10 p.m., Peter Cseh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57680/ > ----------------------------------------------------------- > > (Updated March 22, 2017, 10:10 p.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/main/java/org/apache/oozie/service/sparkconfiguration/SparkConfigReader.java > PRE-CREATION > > core/src/main/java/org/apache/oozie/service/sparkconfiguration/SparkConfigurationStorage.java > PRE-CREATION > core/src/main/resources/oozie-default.xml > 03f428a47e34f2d07ea8fff755f29d222d6aeb9a > > core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java > 1278efa0775c32c16cb38309ab8e92e08d4eb2ec > > core/src/test/java/org/apache/oozie/service/sparkconfiguration/TestSparkConfigurationStorage.java > PRE-CREATION > docs/src/site/twiki/DG_SparkActionExtension.twiki > 863bd89cf576d2fb7be76c61ca139218c33a5f66 > > > Diff: https://reviews.apache.org/r/57680/diff/3/ > > > Testing > ------- > > > Thanks, > > Peter Cseh > >
