----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66930/#review204314 -----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 194 (patched) <https://reviews.apache.org/r/66930/#comment286740> Would handle `NumberFormatException` separately. tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Line 223 (original), 238 (patched) <https://reviews.apache.org/r/66930/#comment286742> Can it be `private`, and if not, is it `@VisibleForTesting`? tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 257 (patched) <https://reviews.apache.org/r/66930/#comment286736> Do we surely need to override `Object#equals()`? tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 267-272 (patched) <https://reviews.apache.org/r/66930/#comment286738> `fs` and `threadPool` should not be part of `equals()`. tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 280 (patched) <https://reviews.apache.org/r/66930/#comment286737> Do we surely need to override `Object#hashCode()`? tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 281-282 (patched) <https://reviews.apache.org/r/66930/#comment286739> `fs` and `threadPool` should not be part of `hashCode()`. tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 290 (patched) <https://reviews.apache.org/r/66930/#comment286741> Can it be `private`, and if not, is it `@VisibleForTesting`? tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 305 (patched) <https://reviews.apache.org/r/66930/#comment286743> Can it be `private`, and if not, is it `@VisibleForTesting`? tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Line 249 (original), 417 (patched) <https://reviews.apache.org/r/66930/#comment286744> Why use `String#format()` here? tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Line 252 (original), 420-422 (patched) <https://reviews.apache.org/r/66930/#comment286745> This should be in `finally`. tools/src/test/java/org/apache/oozie/tools/TestBlockSizeCalculator.java Lines 24 (patched) <https://reviews.apache.org/r/66930/#comment286746> I like this test case :) tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java Lines 31 (patched) <https://reviews.apache.org/r/66930/#comment286747> Would rather not extend `TestOozieSharelibCLI` (because don't want to rerun all those tests once more), but extract the functionality needed from `TestOozieSharelibCLI` and would use rather composition over inheritance in both cases. tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java Lines 38 (patched) <https://reviews.apache.org/r/66930/#comment286748> Would rather not extend `TestOozieSharelibCLI` (because don't want to rerun all those tests once more), but extract the functionality needed from `TestOozieSharelibCLI` and would use rather composition over inheritance in both cases. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java Line 221 (original), 230-236 (patched) <https://reviews.apache.org/r/66930/#comment286749> Would rather extract the functionality needed from `TestOozieSharelibCLI` and would use the extracted class by composition over inheritance in every case. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java Line 240 (original), 255 (patched) <https://reviews.apache.org/r/66930/#comment286750> Would rather extract the functionality needed from `TestOozieSharelibCLI` and would use the extracted class by composition over inheritance in every case. tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java Line 244 (original), 259 (patched) <https://reviews.apache.org/r/66930/#comment286751> Would rather extract the functionality needed from `TestOozieSharelibCLI` and would use the extracted class by composition over inheritance in every case. - András Piros On June 5, 2018, 8:37 a.m., Kinga Marton wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66930/ > ----------------------------------------------------------- > > (Updated June 5, 2018, 8:37 a.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 > > > Diff: https://reviews.apache.org/r/66930/diff/4/ > > > Testing > ------- > > Tested manually > > > Thanks, > > Kinga Marton > >
