> On Dec. 7, 2016, 12:03 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java, lines 234-258 > > <https://reviews.apache.org/r/54383/diff/1/?file=1576464#file1576464line234> > > > > I think `BaseEngine` should contain the three abstract methods > > `killJobs()`, `submitJobs()`, and `resumeJobs()` - we wouldn't have to cast > > 9x. > > > > What moreover bothers me here that we have 3x almost exactly the same > > code - differences are only in the method calls to `? extends BaseEngine`. > > > > My idea is here to have an inner class w/ all the four incoming > > parameters as `final` fields initialized in constructor and having three > > methods like `kill()`, `submit()`, and `resume()` - all the three would > > call the one single `private JSONObject perform(Callable engineCallable) > > throws OozieClientException` that has the `switch case` statements and > > accepts a specific `Callable` that calls `? extends BaseEngine`.
I totally agree that it does not look good. Please consider the 1. All the three methods (killJobs, suspendJobs, and resumeJobs) have different return types: Workflow (WorkflowsInfo), Coordinator (CoordinatorJobInfo), and Bundle (BundleJobInfo). 2. These types do not have any common super hierarchy. Either we need to define a new super type for them or declare them with 'Object' [public abstract Object killJobs(String filter, int start, int len);]. 3. Later these instances needs to be type checked for converting them into their respective JSON format. > On Dec. 7, 2016, 12:03 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java, line 323 > > <https://reviews.apache.org/r/54383/diff/1/?file=1576464#file1576464line323> > > > > I would extract following to a method: > > > > ``` > > private void throwNoOpOozieClientException() throws > > OozieClientException { > > throw new OozieClientException(ErrorCode.E0301.toString(), "no-op"); > > } > > ``` > > > > and wouldn't make any premature optimizations - modern JVMs like > > HotSpot will probably inline that method call anyway. All the methods have different return type, so utility method needs to be defined with returning Object, which later needs to be casted separately. @Override public List<BulkResponse> getBulkInfo(String filter, int start, int len) throws OozieClientException { return (List<BulkResponse>) throwNoOpOozieClientException(); } private Object throwNoOpOozieClientException() throws OozieClientException { throw new OozieClientException(ErrorCode.E0301.toString(), "no-op"); } - Abhishek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54383/#review158328 ----------------------------------------------------------- On Dec. 7, 2016, 4:19 a.m., Abhishek Bafna wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54383/ > ----------------------------------------------------------- > > (Updated Dec. 7, 2016, 4:19 a.m.) > > > 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/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 > > Diff: https://reviews.apache.org/r/54383/diff/ > > > Testing > ------- > > > Thanks, > > Abhishek Bafna > >
