----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62470/#review185905 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 334 (patched) <https://reviews.apache.org/r/62470/#comment262305> It's not very much readable ATM. Can you please call this very method another way like `setupLauncherAndDefaultChildEnv()`, inside call the old `setupLauncherConf()`, and have an empty implementation of `addDefaultChildEnv()` that can be overridden e.g. in `SparkActionExecutor`. core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java Line 101 (original), 102 (patched) <https://reviews.apache.org/r/62470/#comment262306> Please see my comment at `JavaActionExecutor#setupLauncherConf`. If it's implemented the way I propose, this method is not necessary at all. core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java Lines 134-138 (patched) <https://reviews.apache.org/r/62470/#comment262307> You really should have descriptive unit tests covering the changed / added functionality. core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java Lines 136 (patched) <https://reviews.apache.org/r/62470/#comment262308> Why not use only `File.pathSeparator` instead? sharelib/spark/src/test/java/org/apache/oozie/action/hadoop/TestSparkActionExecutor.java Lines 40 (patched) <https://reviews.apache.org/r/62470/#comment262243> Unused import. sharelib/spark/src/test/java/org/apache/oozie/action/hadoop/TestSparkActionExecutor.java Line 107 (original), 108 (patched) <https://reviews.apache.org/r/62470/#comment262302> It seems to me that `_testSetupMethods()` is not gonna be called, so no use in extending that particular test method. Please either make it callable again, or extract to another test method that will be called. sharelib/spark/src/test/java/org/apache/oozie/action/hadoop/TestSparkActionExecutor.java Lines 143 (patched) <https://reviews.apache.org/r/62470/#comment262303> Please always use the forms of `org.junit.Assert.*` that uses a `String message` that shows the caller what's going wrong. - András Piros On Sept. 21, 2017, 4:54 p.m., Satish Saley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62470/ > ----------------------------------------------------------- > > (Updated Sept. 21, 2017, 4:54 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-3062 > https://issues.apache.org/jira/browse/OOZIE-3062 > > > Repository: oozie-git > > > Description > ------- > > OOZIE-3062 Set HADOOP_CONF_DIR for spark action > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > 9d1afb51 > core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java > 80d64ec8 > > sharelib/spark/src/test/java/org/apache/oozie/action/hadoop/TestSparkActionExecutor.java > d97f1f06 > > > Diff: https://reviews.apache.org/r/62470/diff/1/ > > > Testing > ------- > > > Thanks, > > Satish Saley > >
