----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66656/#review201632 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 48 (patched) <https://reviews.apache.org/r/66656/#comment283096> Please put this import to previous group of `java.util.` instead of `org.hadoop.`. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 567-569 (patched) <https://reviews.apache.org/r/66656/#comment283094> Extract method. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 665-668 (patched) <https://reviews.apache.org/r/66656/#comment283105> Use extracted method. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 718 (patched) <https://reviews.apache.org/r/66656/#comment283104> It would be enough to have this method `private final`. Cannot think of any descendant class trying to override that. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 718-740 (patched) <https://reviews.apache.org/r/66656/#comment283106> Please extract to a separate Java class, e.g. `SharelibExcluder`, that would be tested separately. `JavaActionExecutor` is way too heavy by now. Besides, I'm just thinking of some pre-coded blacklist. E.g. so that `oozie*.jar` entries should never be excluded. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 720 (patched) <https://reviews.apache.org/r/66656/#comment283099> `exclusionPattern` core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 726 (patched) <https://reviews.apache.org/r/66656/#comment283093> Some more intuitive error message like `"An unexpected error happened while trying to acquire pattern"` would be way better. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 739 (patched) <https://reviews.apache.org/r/66656/#comment283100> A `DEBUG` level log message is needed here. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 1978-1985 (patched) <https://reviews.apache.org/r/66656/#comment283113> Please extract to a separate Java class, e.g. `SharelibExcluder`, that would be tested separately. `JavaActionExecutor` is way too heavy by now. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 1979 (patched) <https://reviews.apache.org/r/66656/#comment283102> `configuredExclusionValue` core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 1984 (patched) <https://reviews.apache.org/r/66656/#comment283095> `Pattern#compile()` will surely be costly. I'd use a `private static final ConcurrentMap<String, Pattern>`, or [Guava's LoadingCache](https://google.github.io/guava/releases/19.0/api/docs/com/google/common/cache/LoadingCache.html), as sharelib names rarely change. Running such a costly operation twice on a per action basis might affect Oozie server performance if there is a huge load of submitting lots of Oozie applications to YARN in a short period of time. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 1987-1996 (patched) <https://reviews.apache.org/r/66656/#comment283114> Please extract to a separate Java class, e.g. `SharelibExcluder`, that would be tested separately. `JavaActionExecutor` is way too heavy by now. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 1989 (patched) <https://reviews.apache.org/r/66656/#comment283097> I +1 Rohini's opinion on having as exclusion basis the full path of the actual JAR here, not only parent directory name and file name. Imagine following for `actionLibPath`: * `pig/lib/jackson*.jar` * `hbase/lib/jackson*.jar` * `oozie/jackson*.jar` As a user I want following distinct use cases: * exclude `pig/lib/jackson*.jar` only * exclude `pig/lib/jackson*.jar` and `hbase/lib/jackson*.jar` * exclude everything which is `jackson*.jar`, independent of location core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 1990-1992 (patched) <https://reviews.apache.org/r/66656/#comment283101> Rework log message based on `actionLibPath`'s full name. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1030-1061 (patched) <https://reviews.apache.org/r/66656/#comment283112> Please extract to multiple different test cases, one for each condition-expected-actual triplet group. - András Piros On April 17, 2018, 8:29 a.m., Mate Juhasz wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66656/ > ----------------------------------------------------------- > > (Updated April 17, 2018, 8:29 a.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 > bc0f405 > core/src/test/java/org/apache/oozie/service/TestShareLibService.java > d244166 > > > Diff: https://reviews.apache.org/r/66656/diff/1/ > > > Testing > ------- > > Tested on a cluster > > > Thanks, > > Mate Juhasz > >
