----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66656/#review213386 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 176-177 (patched) <https://reviews.apache.org/r/66656/#comment299306> Could you please use 'static final' instead of 'final static' to keep it consistent. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 715 (patched) <https://reviews.apache.org/r/66656/#comment299307> Please add NullPointer check for files core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 755 (patched) <https://reviews.apache.org/r/66656/#comment299308> please insert line break to follow the Oozie coding style core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 757 (patched) <https://reviews.apache.org/r/66656/#comment299309> grammar: Failed to acquire core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 760 (patched) <https://reviews.apache.org/r/66656/#comment299326> Please delete this comment. If it has some valueable information, then extend the log message. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 779 (patched) <https://reviews.apache.org/r/66656/#comment299310> Do we really need this comment? APP_LIB_PATH_LIST = "oozie.wf.application.lib" so I don't see the added value. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 1172 (patched) <https://reviews.apache.org/r/66656/#comment299327> please insert line break to follow the Oozie coding style core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 1175 (patched) <https://reviews.apache.org/r/66656/#comment299311> please insert line break to follow the Oozie coding style core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 41-42 (patched) <https://reviews.apache.org/r/66656/#comment299329> What happens if shareLibRoot is null? It can be null because getSharelibRoot() can return null. In this case we do lots of initialization but shouldExclude will return false. Do I understand it correctly? Do we need the initialization in this case? core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 43 (patched) <https://reviews.apache.org/r/66656/#comment299312> Could we use the same variable name? What is the reason for the rootURI/shareLibRoot name? core/src/main/java/org/apache/oozie/service/ShareLibService.java Lines 624 (patched) <https://reviews.apache.org/r/66656/#comment299313> Please add some error message to eliminate javadoc warnings. core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java Lines 518 (patched) <https://reviews.apache.org/r/66656/#comment299320> I'd rather create two separate methods instead of the boolean contains variable. The new methods could call this (private) method. core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java Lines 521 (patched) <https://reviews.apache.org/r/66656/#comment299314> Please add assert message core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 1 (patched) <https://reviews.apache.org/r/66656/#comment299305> Please add licence information to avoid RAT warning core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 251 (patched) <https://reviews.apache.org/r/66656/#comment299317> please insert line break to follow the Oozie coding style core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 252 (patched) <https://reviews.apache.org/r/66656/#comment299318> Please add assert message core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 253 (patched) <https://reviews.apache.org/r/66656/#comment299319> Please add assert message core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 280 (patched) <https://reviews.apache.org/r/66656/#comment299321> If we need to add three comment lines because this test has 3 important subcases, then we might separate it into 3 methods. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 357 (patched) <https://reviews.apache.org/r/66656/#comment299322> Please avoid javadoc warning core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 432 (patched) <https://reviews.apache.org/r/66656/#comment299323> Please add linebreak core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java Lines 42 (patched) <https://reviews.apache.org/r/66656/#comment299324> typo: EMPTY_ARRAY core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java Lines 54 (patched) <https://reviews.apache.org/r/66656/#comment299325> It's a constant, can we use upper case letters. Can we use a format more similar to the sharelib name used by Oozie? - Andras Salamon On March 1, 2019, 2:44 p.m., Mate Juhasz wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66656/ > ----------------------------------------------------------- > > (Updated March 1, 2019, 2:44 p.m.) > > > Review request for oozie, András Piros and Denes Bodo. > > > Bugs: OOZIE-1624 > https://issues.apache.org/jira/browse/OOZIE-1624 > > > Repository: oozie-git > > > Description > ------- > > OOZIE-1624 Exclusion pattern for sharelib. > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > 231b38ea > core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java > PRE-CREATION > core/src/main/java/org/apache/oozie/service/ShareLibService.java b88dab3a > > core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java > 05511e4c > > core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java > 6383e814 > > core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java > PRE-CREATION > core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/66656/diff/6/ > > > Testing > ------- > > Tested on a cluster > > > Thanks, > > Mate Juhasz > >
