Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/
---

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 

Diff: https://reviews.apache.org/r/54383/diff/


Testing
---


Thanks,

Abhishek Bafna

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

> On Dec. 7, 2016, 12:03 p.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java, lines 234-258
> > 
> >
> > 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
> > 
> >
> > 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 getBulkInfo(String filter, int start, int len) 
throws OozieClientException {
return (List) 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
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Andras Piros]

Hi Abhishek,

please see my refactoring thoughts regarding BaseLocalOozieClient within
the *JIRA issue
*
.

I don't have the right to upload a patch to your ReviewBoard case, hence
over JIRA.

Regards,

Andras

--
Andras PIROS
Software Engineer


On Fri, Dec 9, 2016 at 6:59 AM, Abhishek Bafna  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
> > >  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
> > >  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 getBulkInfo(String filter, int start, int
> len) throws OozieClientException {
> return (List) 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
> >
> >
>
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[András Piros]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review158876
---




core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 35)


Please see 
https://issues.apache.org/jira/secure/attachment/12842828/OOZIE-2751-03.patch 
for my refactoring thoughts.


- András Piros


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
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/
---

(Updated Dec. 16, 2016, 3:32 p.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 (updated)
-

  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/


Testing
---


Thanks,

Abhishek Bafna

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[András Piros]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review159454
---


Ship it!




Ship It!

- András Piros


On Dec. 16, 2016, 3:32 p.m., Abhishek Bafna wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54383/
> ---
> 
> (Updated Dec. 16, 2016, 3:32 p.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/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/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Bafna
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/
---

(Updated Jan. 5, 2017, 5:26 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 (updated)
-

  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/


Testing
---


Thanks,

Abhishek Bafna

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[András Piros]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review160582
---




core/src/main/java/org/apache/oozie/BaseEngine.java (lines 338 - 341)


Why not just call `throwNoOp()` and do not consider its return value? I 
think we don't need this method, as it is mainly a duplicate of `throwNoOp()`.


- András Piros


On Jan. 5, 2017, 5:26 a.m., Abhishek Bafna wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54383/
> ---
> 
> (Updated Jan. 5, 2017, 5:26 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/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/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Bafna
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/
---

(Updated Jan. 5, 2017, 5:34 p.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 (updated)
-

  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/


Testing
---


Thanks,

Abhishek Bafna

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[András Piros]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review160609
---


Ship it!




Ship It!

- András Piros


On Jan. 5, 2017, 5:34 p.m., 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 p.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/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/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Bafna
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Peter Bacsko]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review16
---




core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java
Lines 54 (patched)


Minor: Preconditions.checkNotNull() is useful here IMO.



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java
Lines 68 (patched)


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)


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)


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)


Unnecessary @SuppressWarnings



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java
Lines 345 (patched)


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)


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)


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)


Non-typesafe cast. Do we even need to cast here at all?



core/src/main/java/org/apache/oozie/LocalOozieClientBundle.java
Lines 88 (patched)


Non-typesafe cast. Do we even need to cast here at all?



core/src/main/java/org/apache/oozie/LocalOozieClientCoord.java
Lines 263 (patched)


Non-typesafe cast. Do we even need to cast here at all?



core/src/main/java/org/apache/oozie/LocalOozieClientCoord.java
Lines 272 (patched)


Non-typesafe cast. Do we even need to cast here at all?



core/src/main/java/org/apache/oozie/LocalOozieClientCoord.java
Lines 281 (patched)


Non-typesafe cast. Do we even need to cast here at all?



core/src/main/java/org/apache/oozie/OozieJsonFactory.java
Lines 24 (patched)


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)


Minor: trailing whitespace



core/src/main/java/org/apache/oozie/servlet/V1JobsServlet.java
Line 349 (original), 342 (patched)


Minor: trailing whitespace



core/src/main/java/org/apache/oozie/servlet/V1JobsServlet.java
Line 378 (original), 366 (patched)

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[András Piros]

> On March 24, 2017, 12:24 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java
> > Lines 345 (patched)
> > 
> >
> > Please refactor these nested classes (make them non-nested). The whole 
> > class is too big with these classes being defined here.

+1 on extracting nested classes, and applying SRP.


> On March 24, 2017, 12:24 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java
> > Lines 453 (patched)
> > 
> >
> > 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

Visitor pattern can be heavyweight here, agreed. Renaming `*Visitor` to 
`*Handler` would remove confusion.


- András


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review16
---


On Jan. 5, 2017, 5:34 p.m., 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 p.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/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
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

> On March 24, 2017, 12:24 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java
> > Lines 457 (patched)
> > 
> >
> > Extract cases to constants.
> > 
> > Optional: if it's not too much work, pls also take care of replacing 
> > the other occurrences.

The patch is already big, so it would be better to not touch other 
functionality.


> On March 24, 2017, 12:24 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/LocalOozieClientBundle.java
> > Lines 79 (patched)
> > 
> >
> > Non-typesafe cast. Do we even need to cast here at all?

This is because of super-type vs sub-type. Same as other places.


- Abhishek


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review16
---


On Jan. 5, 2017, 5:34 p.m., 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 p.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/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
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/
---

(Updated April 24, 2017, 1:22 p.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 (updated)
-

  client/src/main/java/org/apache/oozie/client/OozieClient.java 7370808 
  client/src/main/java/org/apache/oozie/client/rest/RestConstants.java f477531 
  core/src/main/java/org/apache/oozie/BaseEngine.java 2780ec2 
  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/OozieClientOperationHandler.java 
PRE-CREATION 
  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/7/

Changes: https://reviews.apache.org/r/54383/diff/6-7/


Testing
---


Thanks,

Abhishek Bafna

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Robert Kanter]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review174067
---




core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java
Lines 92 (patched)


This will cause an NPE if someone tries to use this.  It would be nicer to 
return something like an empty string.  (That's also more consistent with 
getHeaderNames(), which returns an empty iterator).



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java
Lines 461-464 (patched)


Can you move this higher up near the other related methods (e.g. 
getHeader(), getHeaderNames(), etc).  Also, why does this one throw an 
UnsupportedOperationException when the others don't?



core/src/main/java/org/apache/oozie/LocalOozieClientBundle.java
Lines 47 (patched)


Can you add a method to LocalOozie for creating a LocalOozieClientBundle?

If you look at 
https://github.com/apache/oozie/blob/master/core/src/main/java/org/apache/oozie/local/LocalOozie.java,
 you can see there are some methods for creating the WF and Coord local 
clients, but we need to add some for this new bundle local client.

Also please update any Javadocs in LocalOozie that might be out of date.  
There's a lot of lists of NOP methods.


- Robert Kanter


On April 24, 2017, 1:22 p.m., Abhishek Bafna wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54383/
> ---
> 
> (Updated April 24, 2017, 1:22 p.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 7370808 
>   client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 
> f477531 
>   core/src/main/java/org/apache/oozie/BaseEngine.java 2780ec2 
>   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/OozieClientOperationHandler.java 
> PRE-CREATION 
>   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/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Bafna
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/
---

(Updated May 8, 2017, 4:41 p.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 (updated)
-

  client/src/main/java/org/apache/oozie/client/OozieClient.java 7370808 
  client/src/main/java/org/apache/oozie/client/rest/RestConstants.java f477531 
  core/src/main/java/org/apache/oozie/BaseEngine.java 2780ec2 
  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/OozieClientOperationHandler.java 
PRE-CREATION 
  core/src/main/java/org/apache/oozie/OozieJsonFactory.java PRE-CREATION 
  core/src/main/java/org/apache/oozie/local/LocalOozie.java bf1b0db 
  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/8/

Changes: https://reviews.apache.org/r/54383/diff/7-8/


Testing
---


Thanks,

Abhishek Bafna

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Robert Kanter]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review174226
---


Ship it!




Ship It!

- Robert Kanter


On May 8, 2017, 4:41 p.m., Abhishek Bafna wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54383/
> ---
> 
> (Updated May 8, 2017, 4:41 p.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 7370808 
>   client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 
> f477531 
>   core/src/main/java/org/apache/oozie/BaseEngine.java 2780ec2 
>   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/OozieClientOperationHandler.java 
> PRE-CREATION 
>   core/src/main/java/org/apache/oozie/OozieJsonFactory.java PRE-CREATION 
>   core/src/main/java/org/apache/oozie/local/LocalOozie.java bf1b0db 
>   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/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Bafna
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[András Piros]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review158020
---



Some small refactoring advices - but in general, nice job!


core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 38)


Why not make `final`?



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 84)


Collections.emptySet().iterator()



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 229)


Nice to see default path also :)



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (lines 234 - 258)


I would extract this to another - generic - method. The difference is only 
within casting to that generic type, and applying the logic method - that can 
be a `Function` applying `engine.killJobs()` and `getJsonObject()`:


https://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Function.html



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (lines 260 - 284)


I would extract this to another - generic - method. The difference is only 
within casting to that generic type, and applying the logic method - that can 
be a `Function` applying `engine.suspendJobs()` and `getJsonObject()`:


https://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Function.html



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (lines 286 - 310)


I would extract this to another - generic - method. The difference is only 
within casting to that generic type, and applying the logic method - that can 
be a `Function` applying `engine.resumeJobs()` and `getJsonObject()`:


https://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Function.html



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 323)


Extract method



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 328)


Extract method



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 333)


Extract method



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 338)


Extract method



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 343)


Extract method



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 348)


Extract method



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (lines 351 - 376)


Would be better to extract to an Abstract Factory.



core/src/main/java/org/apache/oozie/LocalOozieClientBundle.java (line 49)


I like it :)


- András Piros


On Dec. 5, 2016, 5:05 p.m., Abhishek Bafna wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54383/
> ---
> 
> (Updated Dec. 5, 2016, 5:05 p.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 
> 
> Diff: https://reviews.apache.org/r/54383/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Bafna
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

> On Dec. 5, 2016, 6:15 p.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java, line 84
> > 
> >
> > Collections.emptySet().iterator()

I guess it is the same thing.

public static final  Set emptySet() {
return (Set) EMPTY_SET;
}


> On Dec. 5, 2016, 6:15 p.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java, line 323
> > 
> >
> > Extract method

I believe you mean extract throw statement into a separate method and use the 
method.

I think, it will involde more work (computation) into calling a method and then 
throwing the exception, instead of throwing directly. Thanks.


- Abhishek


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review158020
---


On Dec. 5, 2016, 5:05 p.m., Abhishek Bafna wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54383/
> ---
> 
> (Updated Dec. 5, 2016, 5:05 p.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 
> 
> Diff: https://reviews.apache.org/r/54383/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Bafna
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

> On Dec. 5, 2016, 6:15 p.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java, lines 351-376
> > 
> >
> > Would be better to extract to an Abstract Factory.

Moved them to a separate abstract class and used the same into Servlet classes 
as well. :)


> On Dec. 5, 2016, 6:15 p.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java, lines 234-258
> > 
> >
> > I would extract this to another - generic - method. The difference is 
> > only within casting to that generic type, and applying the logic method - 
> > that can be a `Function` applying `engine.killJobs()` and 
> > `getJsonObject()`:
> > 
> > 
> > https://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Function.html

All these (killJobs, suspendJobs and resumeJobs) are not present in the 
BaseEngine class, that is why type-casting. I might need some more elobration 
on this, otherwise. Thanks.


- Abhishek


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review158020
---


On Dec. 5, 2016, 5:05 p.m., Abhishek Bafna wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54383/
> ---
> 
> (Updated Dec. 5, 2016, 5:05 p.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 
> 
> Diff: https://reviews.apache.org/r/54383/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Bafna
> 
>

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/
---

(Updated Dec. 6, 2016, 6:41 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 (updated)
-

  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

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[Abhishek Bafna]

---
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 (updated)
-

  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

Re: Review Request 54383: LocalOozieClient is missing methods from OozieClient

[András Piros]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54383/#review158328
---




core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (lines 234 - 258)


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`.



core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 323)


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.


- András Piros


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
> 
>