You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Abhishek Bafna <ba...@gmail.com> on 2016/12/05 17:05:03 UTC
Review Request 54383: LocalOozieClient is missing methods from
OozieClient
-----------------------------------------------------------
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
Posted by Abhishek Bafna <ba...@gmail.com>.
> On Dec. 5, 2016, 6:15 p.m., Andr�s Piros wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java, line 84
> > <https://reviews.apache.org/r/54383/diff/1/?file=1576464#file1576464line84>
> >
> > Collections.emptySet().iterator()
I guess it is the same thing.
public static final <T> Set<T> emptySet() {
return (Set<T>) 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
> > <https://reviews.apache.org/r/54383/diff/1/?file=1576464#file1576464line323>
> >
> > 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
Posted by Abhishek Bafna <ba...@gmail.com>.
> On Dec. 5, 2016, 6:15 p.m., Andr�s Piros wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java, lines 351-376
> > <https://reviews.apache.org/r/54383/diff/1/?file=1576464#file1576464line351>
> >
> > 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
> > <https://reviews.apache.org/r/54383/diff/1/?file=1576464#file1576464line234>
> >
> > 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<F, T>` applying `engine.killJobs()` and `getXXXXJsonObject()`:
> >
> > 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
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/54383/#comment228675>
Why not make `final`?
core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 84)
<https://reviews.apache.org/r/54383/#comment228674>
Collections.emptySet().iterator()
core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 229)
<https://reviews.apache.org/r/54383/#comment228684>
Nice to see default path also :)
core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (lines 234 - 258)
<https://reviews.apache.org/r/54383/#comment228686>
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<F, T>` applying `engine.killJobs()` and `getXXXXJsonObject()`:
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)
<https://reviews.apache.org/r/54383/#comment228687>
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<F, T>` applying `engine.suspendJobs()` and `getXXXXJsonObject()`:
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)
<https://reviews.apache.org/r/54383/#comment228688>
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<F, T>` applying `engine.resumeJobs()` and `getXXXXJsonObject()`:
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)
<https://reviews.apache.org/r/54383/#comment228676>
Extract method
core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 328)
<https://reviews.apache.org/r/54383/#comment228677>
Extract method
core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 333)
<https://reviews.apache.org/r/54383/#comment228680>
Extract method
core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 338)
<https://reviews.apache.org/r/54383/#comment228681>
Extract method
core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 343)
<https://reviews.apache.org/r/54383/#comment228682>
Extract method
core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (line 348)
<https://reviews.apache.org/r/54383/#comment228683>
Extract method
core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java (lines 351 - 376)
<https://reviews.apache.org/r/54383/#comment228678>
Would be better to extract to an Abstract Factory.
core/src/main/java/org/apache/oozie/LocalOozieClientBundle.java (line 49)
<https://reviews.apache.org/r/54383/#comment228689>
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
Posted by Andras Piros <an...@cloudera.com>.
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 <ba...@gmail.com> 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
> >
> >
>
>
Re: Review Request 54383: LocalOozieClient is missing methods from
OozieClient
Posted by Abhishek Bafna <ba...@gmail.com>.
> 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
>
>
Re: Review Request 54383: LocalOozieClient is missing methods from
OozieClient
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/54383/#comment229103>
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)
<https://reviews.apache.org/r/54383/#comment229102>
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
>
>
Re: Review Request 54383: LocalOozieClient is missing methods from
OozieClient
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
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
Posted by András Piros <an...@cloudera.com>.
> On March 24, 2017, 12:24 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java
> > Lines 345 (patched)
> > <https://reviews.apache.org/r/54383/diff/6/?file=1597608#file1597608line345>
> >
> > 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)
> > <https://reviews.apache.org/r/54383/diff/6/?file=1597608#file1597608line453>
> >
> > 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/#review169999
-----------------------------------------------------------
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
Posted by Abhishek Bafna <ba...@gmail.com>.
> On March 24, 2017, 12:24 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/BaseLocalOozieClient.java
> > Lines 457 (patched)
> > <https://reviews.apache.org/r/54383/diff/6/?file=1597608#file1597608line457>
> >
> > 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)
> > <https://reviews.apache.org/r/54383/diff/6/?file=1597610#file1597610line79>
> >
> > 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/#review169999
-----------------------------------------------------------
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
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 54383: LocalOozieClient is missing methods from
OozieClient
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
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
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
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
Posted by Abhishek Bafna <ba...@gmail.com>.
-----------------------------------------------------------
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
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/54383/#comment247169>
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)
<https://reviews.apache.org/r/54383/#comment247170>
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)
<https://reviews.apache.org/r/54383/#comment247171>
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
Posted by Abhishek Bafna <ba...@gmail.com>.
-----------------------------------------------------------
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
Posted by Abhishek Bafna <ba...@gmail.com>.
-----------------------------------------------------------
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
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/54383/#comment231715>
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
Posted by Abhishek Bafna <ba...@gmail.com>.
-----------------------------------------------------------
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
Posted by Abhishek Bafna <ba...@gmail.com>.
-----------------------------------------------------------
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
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/54383/#comment229690>
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
Posted by Abhishek Bafna <ba...@gmail.com>.
-----------------------------------------------------------
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
Posted by Abhishek Bafna <ba...@gmail.com>.
-----------------------------------------------------------
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