----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66656/#review203489 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 771-778 (patched) <https://reviews.apache.org/r/66656/#comment285770> I'd have two separate test case in `TestJavaActionExecutor` covering both code paths here. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 776 (patched) <https://reviews.apache.org/r/66656/#comment285755> A `DEBUG` level log here wouldn't hurt. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 17 (patched) <https://reviews.apache.org/r/66656/#comment285756> Please create separate test class `TestShareLibExcluder`. It does not necessarily need to extend `XTestCase`. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 20-22 (patched) <https://reviews.apache.org/r/66656/#comment285745> Have all those fields non-static, only `private final`, and be initialized in the parameterized constructor. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 26 (patched) <https://reviews.apache.org/r/66656/#comment285744> Better to have all this code in the constructor, with the same parameters. Please make all parameters also `final`. Please rename `conf` to something like `actionConf` so that we know what's the difference between `conf` and `jobConf`. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 27 (patched) <https://reviews.apache.org/r/66656/#comment285747> Let also `rootUri` be injected via constructor parameter. `ShareLibExcluder` remains better testable if we don't use any references to `Services` singleton. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 40 (patched) <https://reviews.apache.org/r/66656/#comment285746> Let `Services.get().getConf()` be also injected via constructor parameter, and not invoked in this class. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 49 (patched) <https://reviews.apache.org/r/66656/#comment285748> Having an `INFO` message here would certainly provide more information for the average use cases. core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java Lines 70 (patched) <https://reviews.apache.org/r/66656/#comment285750> `checkExclude()` would be a better name. And please, have it as a non-`static` member method. Also, please use `Preconditions#checkArgument()` to have a better control on incoming parameter correctness. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Line 85 (original), 85 (patched) <https://reviews.apache.org/r/66656/#comment285751> Use `@Override`, maybe? Why not use `private` instead? core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 656 (patched) <https://reviews.apache.org/r/66656/#comment285757> Use `@Override`, maybe? Why not use `private` instead? core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1019 (patched) <https://reviews.apache.org/r/66656/#comment285752> Use `@Override`, maybe? Why not use `private` instead? core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1019-1024 (patched) <https://reviews.apache.org/r/66656/#comment285761> I think you can substitute `createFiles()` call to Guava's `Files#createParentDirs()`, and maybe remove `createFiles()`. I'd rename this method to `createFilesWithParentDirs()`. But anyway, I wouldn't think we need actual `File` instances to test `ShareLibExcluder` functionality that relies on URIs, exclude patterns, and the like. So maybe just remove. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1031 (patched) <https://reviews.apache.org/r/66656/#comment285758> Please move this method to `TestShareLibExcluder`. And give a more expressive name in the format: `testWhenPreconditionsThenExpectedResult()`. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1032 (patched) <https://reviews.apache.org/r/66656/#comment285764> `new Services()` should be covered by `super.setUp()`. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1038 (patched) <https://reviews.apache.org/r/66656/#comment285759> `now` core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1041 (patched) <https://reviews.apache.org/r/66656/#comment285760> I'd rather have an own `DateFormat` instance in the test class. `SimpleDateFormat` is not thread safe - what if we decide one day to run test cases in parallel in the same JVM? core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1060 (patched) <https://reviews.apache.org/r/66656/#comment285767> `services.destroy()` should be covered by `tearDown()`. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1063 (patched) <https://reviews.apache.org/r/66656/#comment285762> Please move this method to `TestShareLibExcluder`. And give a more expressive name in the format: `testWhenPreconditionsThenExpectedResult()`. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1064 (patched) <https://reviews.apache.org/r/66656/#comment285765> `new Services()` should be covered by `super.setUp()`. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1096 (patched) <https://reviews.apache.org/r/66656/#comment285768> `services.destroy()` should be covered by `super.tearDown()`. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1100 (patched) <https://reviews.apache.org/r/66656/#comment285763> Please move this method to `TestShareLibExcluder`. And give a more expressive name in the format: `testWhenPreconditionsThenExpectedResult()`. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1101 (patched) <https://reviews.apache.org/r/66656/#comment285766> `new Services()` should be covered by `super.setUp()`. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Lines 1130 (patched) <https://reviews.apache.org/r/66656/#comment285769> `services.destroy()` should be covered by `super.tearDown()`. core/src/test/java/org/apache/oozie/service/TestShareLibService.java Line 1042 (original), 1162 (patched) <https://reviews.apache.org/r/66656/#comment285753> Use `@Override`, maybe? Why not use `private` instead? - András Piros On May 16, 2018, 3:37 p.m., Mate Juhasz wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66656/ > ----------------------------------------------------------- > > (Updated May 16, 2018, 3:37 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 > 0ba3cbf9 > 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/service/TestShareLibService.java > d2441661 > > > Diff: https://reviews.apache.org/r/66656/diff/2/ > > > Testing > ------- > > Tested on a cluster > > > Thanks, > > Mate Juhasz > >
