Hi Abhishek, please see my refactoring thoughts regarding BaseLocalOozieClient within the *JIRA issue <https://issues.apache.org/jira/secure/attachment/12842828/OOZIE-2751-03.patch>* .
I don't have the right to upload a patch to your ReviewBoard case, hence over JIRA. Regards, Andras -- Andras PIROS Software Engineer <http://www.cloudera.com/> On Fri, Dec 9, 2016 at 6:59 AM, Abhishek Bafna <[email protected]> wrote: > > > > 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 > > > > > >
