----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66656/#review205055 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 35 (patched) <https://reviews.apache.org/r/66656/#comment287935> Should be `static`. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 36 (patched) <https://reviews.apache.org/r/66656/#comment287934> Could be `private`. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 54-62 (patched) <https://reviews.apache.org/r/66656/#comment287936> Remove comment and extract method `Optional<String> tryFindExcludePattern()`. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 64-67 (patched) <https://reviews.apache.org/r/66656/#comment287937> Could use `if (maybeExcludePattern.absent())` instead. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 76 (patched) <https://reviews.apache.org/r/66656/#comment287940> `shouldExclude()` may be a better name. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 81 (patched) <https://reviews.apache.org/r/66656/#comment287938> Does `actionLibURI` always point to a file? core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 85 (patched) <https://reviews.apache.org/r/66656/#comment287939> Does `actionLibURI` always point to a file? core/src/main/java/org/apache/oozie/service/ShareLibService.java Lines 625 (patched) <https://reviews.apache.org/r/66656/#comment287941> A `LOG.debug()` w/ `shareLibRootPath` contents may come on handy here. core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java Lines 334-344 (patched) <https://reviews.apache.org/r/66656/#comment287942> Please create only one implementation w/ a `Collection` type, and call that from the other method. core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java Lines 352-362 (patched) <https://reviews.apache.org/r/66656/#comment287943> Please implement only one method, and call that from another. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java Line 67 (original), 66 (patched) <https://reviews.apache.org/r/66656/#comment287944> Please remove star imports. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 36-50 (patched) <https://reviews.apache.org/r/66656/#comment287950> Use `private static final String[] TEST_SHARELIB_JARS` instead. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 52-61 (patched) <https://reviews.apache.org/r/66656/#comment287949> Use `private static final String[] TEST_SHARELIB_ARCHIVES` instead. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 67-70 (patched) <https://reviews.apache.org/r/66656/#comment287945> Extract method `setHadoopSystemProps()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 71-74 (patched) <https://reviews.apache.org/r/66656/#comment287946> Extract method `createActionConfDirAndFiles()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 78-82 (patched) <https://reviews.apache.org/r/66656/#comment287947> Extract method `createSharelibDirs()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 84-100 (patched) <https://reviews.apache.org/r/66656/#comment287948> Extract method `createSharelibFiles()` and use `foreach` instead. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 110-123 (patched) <https://reviews.apache.org/r/66656/#comment287951> Extract method `createContextUsingSharelib()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 110-120 (patched) <https://reviews.apache.org/r/66656/#comment287976> I feel the need for an `ActionXmlBuilder` class. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 127-132 (patched) <https://reviews.apache.org/r/66656/#comment287952> Extract method `createWorkflowJobUsingSharelib()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 135-140 (patched) <https://reviews.apache.org/r/66656/#comment287953> Extract method `createActionExecutorAndSetupServices()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 146 (patched) <https://reviews.apache.org/r/66656/#comment287954> Add `message`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 148 (patched) <https://reviews.apache.org/r/66656/#comment287955> Add `message`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 152-153 (patched) <https://reviews.apache.org/r/66656/#comment287956> Extract to a well-named method, and remove comment. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 160-161 (patched) <https://reviews.apache.org/r/66656/#comment287957> Extract to a well-named method, and remove comment. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 167-169 (patched) <https://reviews.apache.org/r/66656/#comment287967> Extract to a well-named method, and remove comment. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 175-176 (patched) <https://reviews.apache.org/r/66656/#comment287958> Extract to a well-named method, and remove comment. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 198-203 (patched) <https://reviews.apache.org/r/66656/#comment287959> Use method `createContextUsingSharelib()`. I feel the need for an `ActionXmlBuilder` class. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 198-201 (patched) <https://reviews.apache.org/r/66656/#comment287977> I feel the need for an `ActionXmlBuilder` class. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 208-213 (patched) <https://reviews.apache.org/r/66656/#comment287968> Use method `createWorkflowJobUsingSharelib()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 217-233 (patched) <https://reviews.apache.org/r/66656/#comment287969> Use method `createActionExecutorAndSetupServices()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 269-275 (patched) <https://reviews.apache.org/r/66656/#comment287960> Use method `createContextUsingSharelib()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 269-273 (patched) <https://reviews.apache.org/r/66656/#comment287978> I feel the need for an `ActionXmlBuilder` class. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 277-281 (patched) <https://reviews.apache.org/r/66656/#comment287970> Use method `createActionExecutorAndSetupServices()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 289-293 (patched) <https://reviews.apache.org/r/66656/#comment287974> I feel the need for an `ActionXmlBuilder` class. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 297-301 (patched) <https://reviews.apache.org/r/66656/#comment287971> Use method `createActionExecutorAndSetupServices()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 306-311 (patched) <https://reviews.apache.org/r/66656/#comment287975> I feel the need for an `ActionXmlBuilder` class. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 315-319 (patched) <https://reviews.apache.org/r/66656/#comment287972> Use method `createActionExecutorAndSetupServices()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 364-378 (patched) <https://reviews.apache.org/r/66656/#comment287973> I feel the need for an `ActionXmlBuilder` class. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 384-388 (patched) <https://reviews.apache.org/r/66656/#comment287980> Use method `createActionExecutorAndSetupServices()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 400-414 (patched) <https://reviews.apache.org/r/66656/#comment287979> I feel the need for an `ActionXmlBuilder` class. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 420-424 (patched) <https://reviews.apache.org/r/66656/#comment287981> Use method `createActionExecutorAndSetupServices()`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 433-441 (patched) <https://reviews.apache.org/r/66656/#comment287982> Extract method `checkFilesInClassPath(final Path[] baseClasspath, final Path[] filesToCheck, final Predicate<Boolean> assertion)` core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 442-452 (patched) <https://reviews.apache.org/r/66656/#comment287983> Use method `checkFilesInClassPath(final Path[] baseClasspath, final Path[] filesToCheck, final Predicate<Boolean> assertion)` core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 455-466 (patched) <https://reviews.apache.org/r/66656/#comment287984> Use method `checkFilesInClassPath(final Path[] baseClasspath, final Path[] filesToCheck, final Predicate<Boolean> assertion)` core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java Lines 469-477 (patched) <https://reviews.apache.org/r/66656/#comment287985> Use method `checkFilesInClassPath(final Path[] baseClasspath, final Path[] filesToCheck, final Predicate<Boolean> assertion)` core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java Lines 55-58 (patched) <https://reviews.apache.org/r/66656/#comment287986> `private static final String[] OOZIE_SHARELIB_JARS` core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java Lines 59-62 (patched) <https://reviews.apache.org/r/66656/#comment287987> `private static final String[] SPARK_SHARELIB_JARS` core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java Lines 63-65 (patched) <https://reviews.apache.org/r/66656/#comment287988> `private static final String[] PIG_SHARELIB_JARS` core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java Lines 109 (patched) <https://reviews.apache.org/r/66656/#comment287991> Using an `XLog#debug()` seems to be a better idea. core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java Lines 110 (patched) <https://reviews.apache.org/r/66656/#comment287989> `message` core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java Lines 114 (patched) <https://reviews.apache.org/r/66656/#comment287992> Using an `XLog#debug()` seems to be a better idea. core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java Lines 115 (patched) <https://reviews.apache.org/r/66656/#comment287990> `message` docs/src/site/twiki/WorkflowFunctionalSpec.twiki Lines 2566-2568 (patched) <https://reviews.apache.org/r/66656/#comment287993> The direction is good :) please provide some real-life examples here in the format actual sharelib content, actual configuration, expected HDFS `DistributedCache` content of the `WorkflowActionBean` instance for the use cases: * nothing gets excluded * a simple JAR gets excluded from one sharelib * a range of JARs get excluded from multiple sharelibs based on regexps - András Piros On June 19, 2018, 3:23 p.m., Mate Juhasz wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66656/ > ----------------------------------------------------------- > > (Updated June 19, 2018, 3:23 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 > ed809ef0 > core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java > PRE-CREATION > core/src/main/java/org/apache/oozie/service/ShareLibService.java a901567d > > core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java > f39bba2c > > core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java > a31079a4 > > 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 > docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21e > > > Diff: https://reviews.apache.org/r/66656/diff/3/ > > > Testing > ------- > > Tested on a cluster > > > Thanks, > > Mate Juhasz > >
