----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62350/#review185480 -----------------------------------------------------------
Some comments. Please also add unit tests. tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java Lines 50 (patched) <https://reviews.apache.org/r/62350/#comment261778> We should have log messages for the `if` case when we want to set, and for the now-unsettled `else` case when we don't want to set. tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java Lines 68 (patched) <https://reviews.apache.org/r/62350/#comment261772> Since `Class.forName()` is an expensive operation, it might be worth caching its result. tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java Lines 75 (patched) <https://reviews.apache.org/r/62350/#comment261775> Rename to `getReplicationPolicy()`. tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java Lines 80 (patched) <https://reviews.apache.org/r/62350/#comment261773> Would be better name all the possible `Exception` subclasses that may arise. tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java Lines 82 (patched) <https://reviews.apache.org/r/62350/#comment261774> Since we don't throw `RuntimeException` within `isHadoop3()`, and the first part of this method is covered there, I suggest having a common method `getECPoliciesClass()` that throws `ClassNotFoundException` and is used by both `isHadoop3()` and `getReplicationPolicy()`. tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java Lines 103 (patched) <https://reviews.apache.org/r/62350/#comment261776> `System.err.println()` tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java Lines 112 (patched) <https://reviews.apache.org/r/62350/#comment261777> `System.err.println()` - András Piros On Sept. 15, 2017, 12:07 p.m., Peter Bacsko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62350/ > ----------------------------------------------------------- > > (Updated Sept. 15, 2017, 12:07 p.m.) > > > Review request for oozie, András Piros, Peter Cseh, and Robert Kanter. > > > Repository: oozie-git > > > Description > ------- > > See https://issues.apache.org/jira/browse/OOZIE-3054 > > > Diffs > ----- > > tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java PRE-CREATION > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java a844aa0f8 > > > Diff: https://reviews.apache.org/r/62350/diff/1/ > > > Testing > ------- > > > Thanks, > > Peter Bacsko > >
