----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66930/#review204384 -----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 337-341 (patched) <https://reviews.apache.org/r/66930/#comment286871> Extract parameters to local variables for better readability. tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java Lines 35 (patched) <https://reviews.apache.org/r/66930/#comment286877> When we have `JUnit4Runner` (have we?) we can use `@Rule public TemporaryFolder tmpFolder;` instead. tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java Lines 43 (patched) <https://reviews.apache.org/r/66930/#comment286875> Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor. tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java Lines 51 (patched) <https://reviews.apache.org/r/66930/#comment286874> Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor, and call e.g. `services.destroy()` explicitly here. tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java Lines 55 (patched) <https://reviews.apache.org/r/66930/#comment286876> I don't get from the unit test name alone what it's supposed to test and what is the expectation. tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java Lines 42 (patched) <https://reviews.apache.org/r/66930/#comment286879> When we have `JUnit4Runner` (have we?) we can use `@Rule public TemporaryFolder tmpFolder;` instead. tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java Lines 44 (patched) <https://reviews.apache.org/r/66930/#comment286882> Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor. tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java Lines 57 (patched) <https://reviews.apache.org/r/66930/#comment286884> Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor, and call e.g. `services.destroy()` explicitly here. tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java Lines 61 (patched) <https://reviews.apache.org/r/66930/#comment286878> Same as above - please provide a more describing name. It would be great to have different unit test cases with and without an own `ExecutorService`, testing with a single thread / many concurrent threads, and covering the previously failing functionality as well. tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java Lines 64-65 (patched) <https://reviews.apache.org/r/66930/#comment286881> Copying more files per thread can also be a different test case. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java Line 51 (original), 39 (patched) <https://reviews.apache.org/r/66930/#comment286880> When we have `JUnit4Runner` (have we?) we can use `@Rule public TemporaryFolder tmpFolder;` instead. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java Lines 41 (patched) <https://reviews.apache.org/r/66930/#comment286883> Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java Lines 58 (patched) <https://reviews.apache.org/r/66930/#comment286885> Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor, and call e.g. `services.destroy()` explicitly here. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java Lines 35 (patched) <https://reviews.apache.org/r/66930/#comment286870> `OozieSharelibFileOperations` is a better name. It's not a test class, and I don't like the words `Helper`, `Util`, and the like as class or method names. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java Lines 38-40 (patched) <https://reviews.apache.org/r/66930/#comment286869> Would have all these as `private final`, and injected via constructor. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java Lines 81-90 (patched) <https://reviews.apache.org/r/66930/#comment286867> Would rather inject a `FileSystem` instance from the outside and `init()` it from the caller class' `setUp()` method. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java Lines 92-103 (patched) <https://reviews.apache.org/r/66930/#comment286866> Would rather inject a `Services` instance from the outside and `init()` it from the caller class' `setUp()` method. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java Lines 105-109 (patched) <https://reviews.apache.org/r/66930/#comment286865> Would rather inject a `Services` instance from the outside and `destroy()` it from the caller class' `tearDown()` method. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java Lines 111-117 (patched) <https://reviews.apache.org/r/66930/#comment286868> Would rather inject `systemLibPath` from the outside. - András Piros On June 6, 2018, 12:34 p.m., Kinga Marton wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66930/ > ----------------------------------------------------------- > > (Updated June 6, 2018, 12:34 p.m.) > > > Review request for oozie, András Piros and Peter Cseh. > > > Repository: oozie-git > > > Description > ------- > > On a busy Hadoop cluster it can happen that users cannot install properly > Oozie ShareLib. > > > Diffs > ----- > > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java dce1c559 > tools/src/test/java/org/apache/oozie/tools/TestBlockSizeCalculator.java > PRE-CREATION > tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java > PRE-CREATION > tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java > PRE-CREATION > tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java > ccad273b > tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/66930/diff/5/ > > > Testing > ------- > > Tested manually > > > Thanks, > > Kinga Marton > >
