----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54383/#review169999 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java Lines 54 (patched) <https://reviews.apache.org/r/54383/#comment242725> Minor: Preconditions.checkNotNull() is useful here IMO. core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java Lines 68 (patched) <https://reviews.apache.org/r/54383/#comment242722> Is this method ever called? If not, it should be abstract. If it is, please add a small "// no-op" comment to indicate that it is a valid implementation. core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java Lines 82 (patched) <https://reviews.apache.org/r/54383/#comment242723> Is this method ever called? If not, it should be abstract. If it is, please add a small "// no-op" comment to indicate that it is a valid implementation. core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java Lines 91 (patched) <https://reviews.apache.org/r/54383/#comment242724> Is this method ever called? If not, it should be abstract. If it is, please add a small "// no-op" comment to indicate that it is a valid implementation. core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java Lines 95 (patched) <https://reviews.apache.org/r/54383/#comment242727> Unnecessary @SuppressWarnings core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java Lines 345 (patched) <https://reviews.apache.org/r/54383/#comment242726> Please refactor these nested classes (make them non-nested). The whole class is too big with these classes being defined here. core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java Lines 453 (patched) <https://reviews.apache.org/r/54383/#comment242739> As I can see it, this is not the visitor pattern. Traditionally, when you use the visitor pattern, the classes that are to be visited implement the accept() method which takes the visitor implementation as an argument. Then, if the class contains other classes that can be visited, it calls the accept() method on the embedded object again, effectively passing the visitor implementation down the call stack. Also, the visitation begins by calling the accept() method on the top-most (external) object. There is a very good, simple example here: https://en.wikipedia.org/wiki/Visitor_pattern#Java_example I think this is just having different implementations, which are hidden under an interface (which is good)/ I suggest doing the following rename: - OozieActionVisitor --> OozieActionHandler - AbstractOozieActionVisitor --> AbstractOozieActionHandler - Killing/Resuming/SuspendingVisitor --> Killing/Resuming/SuspendingHandler core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java Lines 457 (patched) <https://reviews.apache.org/r/54383/#comment242728> Extract cases to constants. Optional: if it's not too much work, pls also take care of replacing the other occurrences. core/src/main/java/org/apache/oozie/LocalOozieClientBundle.java Lines 79 (patched) <https://reviews.apache.org/r/54383/#comment242731> Non-typesafe cast. Do we even need to cast here at all? core/src/main/java/org/apache/oozie/LocalOozieClientBundle.java Lines 88 (patched) <https://reviews.apache.org/r/54383/#comment242730> Non-typesafe cast. Do we even need to cast here at all? core/src/main/java/org/apache/oozie/LocalOozieClientCoord.java Lines 263 (patched) <https://reviews.apache.org/r/54383/#comment242732> Non-typesafe cast. Do we even need to cast here at all? core/src/main/java/org/apache/oozie/LocalOozieClientCoord.java Lines 272 (patched) <https://reviews.apache.org/r/54383/#comment242733> Non-typesafe cast. Do we even need to cast here at all? core/src/main/java/org/apache/oozie/LocalOozieClientCoord.java Lines 281 (patched) <https://reviews.apache.org/r/54383/#comment242734> Non-typesafe cast. Do we even need to cast here at all? core/src/main/java/org/apache/oozie/OozieJsonFactory.java Lines 24 (patched) <https://reviews.apache.org/r/54383/#comment242735> If it's meant to be an utility class, then it should be final instead of abstract. Also, consider adding a private constuctor to prevent instantiation. core/src/main/java/org/apache/oozie/servlet/V1JobsServlet.java Line 316 (original), 314 (patched) <https://reviews.apache.org/r/54383/#comment242736> Minor: trailing whitespace core/src/main/java/org/apache/oozie/servlet/V1JobsServlet.java Line 349 (original), 342 (patched) <https://reviews.apache.org/r/54383/#comment242737> Minor: trailing whitespace core/src/main/java/org/apache/oozie/servlet/V1JobsServlet.java Line 378 (original), 366 (patched) <https://reviews.apache.org/r/54383/#comment242738> Minor: trailing whitespace - Peter Bacsko On jan. 5, 2017, 5:34 du, Abhishek Bafna wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54383/ > ----------------------------------------------------------- > > (Updated jan. 5, 2017, 5:34 du) > > > Review request for oozie. > > > Bugs: OOZIE-2751 > https://issues.apache.org/jira/browse/OOZIE-2751 > > > Repository: oozie-git > > > Description > ------- > > LocalOozieClient is missing methods from OozieClient > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/client/OozieClient.java 12c80cb > core/src/main/java/org/apache/oozie/BaseEngine.java 50df897 > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java PRE-CREATION > core/src/main/java/org/apache/oozie/LocalOozieClient.java f734f76 > core/src/main/java/org/apache/oozie/LocalOozieClientBundle.java > PRE-CREATION > core/src/main/java/org/apache/oozie/LocalOozieClientCoord.java 32b0cd0 > core/src/main/java/org/apache/oozie/OozieJsonFactory.java PRE-CREATION > core/src/main/java/org/apache/oozie/servlet/V0JobsServlet.java 2c79ef0 > core/src/main/java/org/apache/oozie/servlet/V1JobsServlet.java 80c8ec4 > core/src/test/java/org/apache/oozie/TestLocalOozieClientCoord.java 4decd52 > > > Diff: https://reviews.apache.org/r/54383/diff/6/ > > > Testing > ------- > > > Thanks, > > Abhishek Bafna > >
