----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66930/#review202354 -----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 224-235 (patched) <https://reviews.apache.org/r/66930/#comment284133> Please extract to another `static final class`, and add integration tests that use `XTestCase` to check copying real data onto `MiniDFSCluster`. tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 226 (patched) <https://reviews.apache.org/r/66930/#comment284125> Would be way nicer to have this `ExecutorService` as `private final` field. tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 233 (patched) <https://reviews.apache.org/r/66930/#comment284126> A log message to note when all the tasks are done would be nice. tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 245-260 (patched) <https://reviews.apache.org/r/66930/#comment284130> Please extract this `Callable` to another `private static final class ... extends Callable<CopyTaskConfiguration>` tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 252-254 (patched) <https://reviews.apache.org/r/66930/#comment284131> Please extract parameters to well-named local variables. tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 266-276 (patched) <https://reviews.apache.org/r/66930/#comment284132> Please extract to `static class BlockSizeCalculator`, and add unit tests covering its functionality. tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 303 (patched) <https://reviews.apache.org/r/66930/#comment284134> `System.err.println(e.getMessage())` instead. tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 307 (patched) <https://reviews.apache.org/r/66930/#comment284135> `System.err.println("Retrying to copy ...")` tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 322 (patched) <https://reviews.apache.org/r/66930/#comment284136> I'm just wondering whether in case any errors we should 1) rollback the stuff we halfway managed to copy 2) die after this `System.err.println()` call. tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 349-351 (patched) <https://reviews.apache.org/r/66930/#comment284137> I'm just wondering whether in case any errors we should 1) rollback the stuff we halfway managed to copy 2) die after this `System.err.println()` call. tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Line 259 (original), 355 (patched) <https://reviews.apache.org/r/66930/#comment284127> `private final static class` tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 260-262 (original), 356-359 (patched) <https://reviews.apache.org/r/66930/#comment284128> `private final` tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Line 264 (original), 361 (patched) <https://reviews.apache.org/r/66930/#comment284129> `private` tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java Lines 370-377 (patched) <https://reviews.apache.org/r/66930/#comment284138> Please add curly braces to this autogenerated method. - András Piros On May 3, 2018, 10:07 a.m., Kinga Marton wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66930/ > ----------------------------------------------------------- > > (Updated May 3, 2018, 10:07 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 dce1c559b > > > Diff: https://reviews.apache.org/r/66930/diff/1/ > > > Testing > ------- > > Tested manually > > > Thanks, > > Kinga Marton > >
