----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57680/#review169633 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/service/SparkConfigurationService.java Line 53 (original), 40 (patched) <https://reviews.apache.org/r/57680/#comment242106> This can just be ````= services.get(ConfigurationService.class)```` core/src/main/java/org/apache/oozie/service/sparkconfiguration/SparkConfigReader.java Lines 75 (patched) <https://reviews.apache.org/r/57680/#comment242109> If there's a problem loading the properties, will you end up with a messed up ````props````? e.g. it loads some of the properties or you have corrupted properties? It's probably safest to clear out all propeties in the catch to make sure. core/src/main/java/org/apache/oozie/service/sparkconfiguration/SparkConfigurationStorage.java Lines 43-44 (patched) <https://reviews.apache.org/r/57680/#comment242111> I think we should enforce that at least one "." and one character is here. As it is now, ````oozie.service.SparkConfigurationService.configurations```` will pass this regex. Please double check, but I believe this regex will work: ```` "^"+SparkConfigurationService.SPARK_CONFIGURATION_PREFIX + "\..+" + CONFIGURATIONS_SUFFIX + "$" ```` It enforces a period followed by at least one character. core/src/main/java/org/apache/oozie/service/sparkconfiguration/SparkConfigurationStorage.java Lines 47 (patched) <https://reviews.apache.org/r/57680/#comment242118> Can you add a comment explaining this datatype? e.g. "This is a mapping from RM host:ports to a map of sharelib names to spark properties". That will help future people understand this better. core/src/main/java/org/apache/oozie/service/sparkconfiguration/SparkConfigurationStorage.java Lines 71 (patched) <https://reviews.apache.org/r/57680/#comment242116> toLowerCase() core/src/main/resources/oozie-default.xml Lines 2870-2881 (original), 2870-2881 (patched) <https://reviews.apache.org/r/57680/#comment242133> We should update the description to explain the new sharelib changes. core/src/main/resources/oozie-default.xml Lines 2870-2881 (original), 2870-2881 (patched) <https://reviews.apache.org/r/57680/#comment242138> We should update the description for this to mention that you can use other sharelib names. core/src/main/resources/oozie-default.xml Lines 2883-2890 (original), 2883-2890 (patched) <https://reviews.apache.org/r/57680/#comment242132> We should update the description for this to mention that you can use other sharelib names. core/src/main/resources/oozie-default.xml Lines 2883-2892 (original), 2883-2892 (patched) <https://reviews.apache.org/r/57680/#comment242139> We should update the description to explain the new sharelib changes. core/src/main/resources/oozie-default.xml Line 2891 (original), 2891 (patched) <https://reviews.apache.org/r/57680/#comment242130> It would also be good to update the twiki docs with this info too. We already mention the Spark on Yarn configs here: http://oozie.apache.org/docs/4.3.0/DG_SparkActionExtension.html#Spark_on_YARN core/src/main/resources/oozie-default.xml Line 2892 (original), 2892 (patched) <https://reviews.apache.org/r/57680/#comment242134> The twiki docs mention the Spark configuration here: http://oozie.apache.org/docs/4.3.0/DG_SparkActionExtension.html#Spark_on_YARN We should probably also update there as well. core/src/main/resources/oozie-default.xml Lines 2893-2903 (original) <https://reviews.apache.org/r/57680/#comment242127> Shouldn't we leave this here for now? core/src/main/resources/oozie-default.xml Lines 2893-2903 (original) <https://reviews.apache.org/r/57680/#comment242136> We should keep this, right? - Robert Kanter On March 21, 2017, 1:50 p.m., Peter Cseh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57680/ > ----------------------------------------------------------- > > (Updated March 21, 2017, 1:50 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 > b481887122be0112c763ad667d1de18747d1ad9b > > core/src/test/java/org/apache/oozie/service/TestSparkConfigurationService.java > 0e00a457890d1f4b8e86c19043cd597928cdd9bb > > core/src/test/java/org/apache/oozie/service/sparkconfiguration/TestSparkConfigurationStorage.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/57680/diff/2/ > > > Testing > ------- > > > Thanks, > > Peter Cseh > >
