Re: Review Request 69916: OOZIE-3427 - Use best practices in HTTP response headers

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Feb. 7, 2019, 3:48 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69916/
> ---
> 
> (Updated Feb. 7, 2019, 3:48 p.m.)
> 
> 
> Review request for oozie, Denes Bodo, Kinga Marton, and Mate Juhasz.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3427 - Use best practices in HTTP response headers
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/servlet/HttpResponseHeaderFilter.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/oozie/servlet/DagServletTestCase.java 
> ce731a16b 
>   
> core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
>  bc469e260 
>   
> core/src/test/java/org/apache/oozie/servlet/TestBulkMonitorWebServiceAPI.java 
> b4054b05f 
>   
> core/src/test/java/org/apache/oozie/servlet/TestHttpResponseHeaderFilter.java 
> PRE-CREATION 
>   server/src/main/java/org/apache/oozie/server/FilterMapper.java 3dc9be815 
>   server/src/test/java/org/apache/oozie/server/TestEmbeddedOozieServer.java 
> 58543e6fd 
>   webapp/src/main/webapp/WEB-INF/web.xml 2edbdf153 
> 
> 
> Diff: https://reviews.apache.org/r/69916/diff/2/
> 
> 
> Testing
> ---
> 
> Tested embedded jetty and war installed to a local tomcat. Local tomcat was 
> hardly working, but at least I was able to test /versions.
> 
> $ wget -qS http://localhost:11000/oozie/index.jsp
> HTTP/1.1 200 OK
>   Date: Thu, 07 Feb 2019 09:44:32 GMT
>   X-Frame-Options: DENY
>   Content-Type: text/html;charset=utf-8
>   Set-Cookie: JSESSIONID=1lx0y9fy2pd6n1rh911vc2l1sd;Path=/oozie
>   Expires: Thu, 01 Jan 1970 00:00:00 GMT
>   Content-Length: 3739
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 69916: OOZIE-3427 - Use best practices in HTTP response headers

[András Piros via Review Board]

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



Changes look good to me. Waiting on Zsombor Gegesy's last review comment to 
clean up.

- András Piros


On Feb. 7, 2019, 3:48 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69916/
> ---
> 
> (Updated Feb. 7, 2019, 3:48 p.m.)
> 
> 
> Review request for oozie, Denes Bodo, Kinga Marton, and Mate Juhasz.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3427 - Use best practices in HTTP response headers
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/servlet/HttpResponseHeaderFilter.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/oozie/servlet/DagServletTestCase.java 
> ce731a16b 
>   
> core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
>  bc469e260 
>   
> core/src/test/java/org/apache/oozie/servlet/TestBulkMonitorWebServiceAPI.java 
> b4054b05f 
>   
> core/src/test/java/org/apache/oozie/servlet/TestHttpResponseHeaderFilter.java 
> PRE-CREATION 
>   server/src/main/java/org/apache/oozie/server/FilterMapper.java 3dc9be815 
>   server/src/test/java/org/apache/oozie/server/TestEmbeddedOozieServer.java 
> 58543e6fd 
>   webapp/src/main/webapp/WEB-INF/web.xml 2edbdf153 
> 
> 
> Diff: https://reviews.apache.org/r/69916/diff/2/
> 
> 
> Testing
> ---
> 
> Tested embedded jetty and war installed to a local tomcat. Local tomcat was 
> hardly working, but at least I was able to test /versions.
> 
> $ wget -qS http://localhost:11000/oozie/index.jsp
> HTTP/1.1 200 OK
>   Date: Thu, 07 Feb 2019 09:44:32 GMT
>   X-Frame-Options: DENY
>   Content-Type: text/html;charset=utf-8
>   Set-Cookie: JSESSIONID=1lx0y9fy2pd6n1rh911vc2l1sd;Path=/oozie
>   Expires: Thu, 01 Jan 1970 00:00:00 GMT
>   Content-Length: 3739
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 69916: OOZIE-3427 - Use best practices in HTTP response headers

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/servlet/HTTPResponseFilter.java
Lines 36 (patched)


Please give this a more descriptive and specific name like 
`HttpResponseHeadersFilter`.



core/src/main/java/org/apache/oozie/servlet/HTTPResponseFilter.java
Lines 69-70 (patched)


Shouldn't we first call `chain.doFilter()`, and then add the response 
header(s) to possibly avoid setting them to `null` in a later call down the 
filter chain?



core/src/test/java/org/apache/oozie/servlet/TestHTTPResponseFilter.java
Lines 36 (patched)


While this only test case is OK with me, can you please add more to this:

* clickjacking attempt should result unsuccessful
* normal HTTP servlet, e.g. `VersionServlet`, gives also this HTTP response 
header



core/src/test/java/org/apache/oozie/servlet/TestHTTPResponseFilter.java
Lines 39 (patched)


Please rename to `testXFrameOptionAgainstClickjackingAdded()`.



core/src/test/java/org/apache/oozie/servlet/TestHTTPResponseFilter.java
Lines 44 (patched)


Why do we need `AtomicBoolean` here?


- András Piros


On Feb. 7, 2019, 12:40 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69916/
> ---
> 
> (Updated Feb. 7, 2019, 12:40 p.m.)
> 
> 
> Review request for oozie, Denes Bodo, Kinga Marton, and Mate Juhasz.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3427 - Use best practices in HTTP response headers
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/servlet/HTTPResponseFilter.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/oozie/servlet/TestHTTPResponseFilter.java 
> PRE-CREATION 
>   server/src/main/java/org/apache/oozie/server/FilterMapper.java 3dc9be815 
>   webapp/src/main/webapp/WEB-INF/web.xml 2edbdf153 
> 
> 
> Diff: https://reviews.apache.org/r/69916/diff/1/
> 
> 
> Testing
> ---
> 
> Tested embedded jetty and war installed to a local tomcat. Local tomcat was 
> hardly working, but at least I was able to test /versions.
> 
> $ wget -qS http://localhost:11000/oozie/index.jsp
> HTTP/1.1 200 OK
>   Date: Thu, 07 Feb 2019 09:44:32 GMT
>   X-Frame-Options: DENY
>   Content-Type: text/html;charset=utf-8
>   Set-Cookie: JSESSIONID=1lx0y9fy2pd6n1rh911vc2l1sd;Path=/oozie
>   Expires: Thu, 01 Jan 1970 00:00:00 GMT
>   Content-Length: 3739
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 69657: OOZIE-3407 - Cleanup TestPurgeXCommand

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Jan. 10, 2019, 1:26 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69657/
> ---
> 
> (Updated Jan. 10, 2019, 1:26 p.m.)
> 
> 
> Review request for oozie, András Piros and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3407 - Cleanup TestPurgeXCommand
> 
> 
> Diffs
> -
> 
>   core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java 
> 107547d87d294d600986d361ea1a6f92298e97f7 
> 
> 
> Diff: https://reviews.apache.org/r/69657/diff/2/
> 
> 
> Testing
> ---
> 
> Running unit tests localy
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 69657: OOZIE-3407 - Cleanup TestPurgeXCommand

[András Piros via Review Board]

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



The patch does a great job into the right direction.

Only a few nit-like comments:

* test method naming
* separate out test classes like `TestWorkflowPurgeXCommand`, 
`TestCoordinatorPurgeXCommand`, `TestBundlePurgeXCommand`
* apply e.g. the builder pattern to set up test bed at the beginning of each 
test method / at the end when it comes to assertions
* separate out calls to `new PurgeXCommand(...).call()` with descriptive method 
names


core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 82 (original), 90 (patched)


Is it absolutely necessary to have yet another `Services` reference? Wasn't 
the existing `protected Services services` enough / working properly?



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 90 (original), 98 (patched)


Is it absolutely necessary to have yet another `Services` reference? Wasn't 
the existing `protected Services services` enough / working properly?



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 103 (original), 111 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 144 (original), 126 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 185 (original), 141 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 226 (original), 156 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 266 (original), 171 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 306 (original), 187 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 346 (original), 203 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 386 (original), 219 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 423 (original), 235 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 476 (original), 257 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 529 (original), 280 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 582 (original), 302 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 631 (original), 324 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 693 (original), 347 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 758 (original), 370 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 818 (original), 391 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 879 (original), 413 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Line 1075 (original), 439 (patched)


Please provide a more descriptive name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java

Re: Review Request 69594: OOZIE-3400: Fix PurgeService sub-sub-workflow checking

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Dec. 27, 2018, 3:13 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69594/
> ---
> 
> (Updated Dec. 27, 2018, 3:13 p.m.)
> 
> 
> Review request for oozie, András Piros and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3400: Fix PurgeService sub-sub-workflow checking
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 9cc153bb0 
>   core/src/main/java/org/apache/oozie/command/PurgeXCommand.java 42c3b28a6 
>   core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java 
> d11fcffbb 
>   core/src/test/java/org/apache/oozie/command/TestSelectorTreeTraverser.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69594/diff/3/
> 
> 
> Testing
> ---
> 
> Run TestPurgeXCommand unit tests locally.
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 69594: OOZIE-3400: Fix PurgeService sub-sub-workflow checking

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/command/PurgeXCommand.java
Lines 75-78 (patched)


Can we express it JDK7-like? Maybe using [Guava's 
`Function`](https://github.com/google/guava/wiki/FunctionalExplained)?



core/src/main/java/org/apache/oozie/command/PurgeXCommand.java
Lines 81 (patched)


What's the semantics of `T` and `U`?



core/src/main/java/org/apache/oozie/command/PurgeXCommand.java
Lines 82-84 (patched)


Can be `final`



core/src/main/java/org/apache/oozie/command/PurgeXCommand.java
Lines 86 (patched)


Can all be `final`



core/src/main/java/org/apache/oozie/command/PurgeXCommand.java
Lines 93-94 (patched)


Please apply a more differentiated naming on both `List` and `Set` 
instances. Looking only at their names isn't obvious which holds what.



core/src/main/java/org/apache/oozie/command/PurgeXCommand.java
Lines 105-107 (patched)


So you're checking whether there was a duplicate... I think it's a lot of 
`equals()` call when speaking of a `Set#addAll()` in a `while` loop.

Do you think we need optimizing on that? About how many times is 
`Set#addAll()` called?



core/src/main/java/org/apache/oozie/command/PurgeXCommand.java
Lines 109-111 (patched)


When not all the descendant nodes were selected, we don't select any on 
this level, right?



core/src/main/java/org/apache/oozie/command/PurgeXCommand.java
Lines 256 (patched)


Can we express it JDK7-like? Maybe using [Guava's 
`Function`](https://github.com/google/guava/wiki/FunctionalExplained)?



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Lines 2881 (patched)


Please extract well-named variables.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Lines 2908 (patched)


Please extract well-named variables.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Lines 2939 (patched)


Please extract well-named variables.



core/src/test/java/org/apache/oozie/command/TestSelectorTreeTraverser.java
Lines 37-61 (patched)


Can we express it JDK7-like? Maybe using [Guava's 
`Function`](https://github.com/google/guava/wiki/FunctionalExplained)?


- András Piros


On Dec. 21, 2018, 3:17 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69594/
> ---
> 
> (Updated Dec. 21, 2018, 3:17 p.m.)
> 
> 
> Review request for oozie, András Piros and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3400: Fix PurgeService sub-sub-workflow checking
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 9cc153bb0 
>   core/src/main/java/org/apache/oozie/command/PurgeXCommand.java 42c3b28a6 
>   core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java 
> d11fcffbb 
>   core/src/test/java/org/apache/oozie/command/TestSelectorTreeTraverser.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69594/diff/2/
> 
> 
> Testing
> ---
> 
> Run TestPurgeXCommand unit tests locally.
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 69594: OOZIE-3400: Fix PurgeService sub-sub-workflow checking

[András Piros via Review Board]

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



Nice job so far. I really would rather extract functionality into a nested 
class inside `PurgeXCommand`, and test it separately, for better readability 
and SRP.


core/src/main/java/org/apache/oozie/command/PurgeXCommand.java
Lines 212-213 (original), 214-232 (patched)


I'm wondering whether it covers an arbitrarily deep `WorkflowJobBean` -> 
`WorkflowActionBean` -> `WorkflowJobBean` structure.

If so, are we checking for DAGness anywhere?



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Lines 2871 (patched)


Please give a more descriptive test method name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Lines 2898 (patched)


Please give a more descriptive test method name.



core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java
Lines 2925 (patched)


Please give a more descriptive test method name.


- András Piros


On Dec. 19, 2018, 9:57 a.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69594/
> ---
> 
> (Updated Dec. 19, 2018, 9:57 a.m.)
> 
> 
> Review request for oozie, András Piros and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3400: Fix PurgeService sub-sub-workflow checking
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/command/PurgeXCommand.java 42c3b28a6 
>   core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java 
> d11fcffbb 
> 
> 
> Diff: https://reviews.apache.org/r/69594/diff/1/
> 
> 
> Testing
> ---
> 
> Run TestPurgeXCommand unit tests locally.
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Dec. 4, 2018, 5:29 p.m., zhang junfan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69330/
> ---
> 
> (Updated Dec. 4, 2018, 5:29 p.m.)
> 
> 
> Review request for oozie, András Piros and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Auth token cache file name should include Oozie URL, link to oozie-3379.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
>   core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
> fc9d840b 
>   
> core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
>  04fde730 
> 
> 
> Diff: https://reviews.apache.org/r/69330/diff/4/
> 
> 
> Testing
> ---
> 
> I added a test for authOozieClients with the different oozieUrl.
> I changed the code for EmbeddedServletContainer. Because this patch needs to 
> bind the cache file to oozieUrl, you need to test whether multiple different 
> clients can use the cache. In the original EmbeddedServletContainer code, the 
> port was random and could not get a given container context path. So the port 
> binding needs to be added.
> 
> 
> File Attachments
> 
> 
> oozie-3379-6.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch
> 
> 
> Thanks,
> 
> zhang junfan
> 
>

Re: Review Request 69492: OOZIE-3389 Getting input dependency list on the UI throws NPE

[András Piros via Review Board]

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

(Updated Dec. 1, 2018, 2:21 p.m.)


Review request for oozie, Andras Salamon and Kinga Marton.


Changes
---

Addressing review comments.


Repository: oozie-git


Description
---

OOZIE-3389 Getting input dependency list on the UI throws NPE


Diffs (updated)
-

  
core/src/main/java/org/apache/oozie/command/coord/CoordActionMissingDependenciesXCommand.java
 d37cfe5125a5c3a665b3e33e6ac3fd5581fe278f 
  
core/src/main/java/org/apache/oozie/coord/input/dependency/CoordOldInputDependency.java
 56aef1c1779ed7446852af42f25c5ca058e473e4 
  
core/src/test/java/org/apache/oozie/coord/input/dependency/TestCoordOldInputDependency.java
 PRE-CREATION 
  core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java 
bacfe89bb3d836da1f6e559fa68a5bf4db970b09 


Diff: https://reviews.apache.org/r/69492/diff/2/

Changes: https://reviews.apache.org/r/69492/diff/1-2/


Testing
---

Introduced new unit test class `TestCoordOldInputDependency`.


Thanks,

András Piros

Re: Review Request 69492: OOZIE-3389 Getting input dependency list on the UI throws NPE

[András Piros via Review Board]

> On Nov. 30, 2018, 1:01 p.m., Andras Salamon wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Line 121 (original), 120 (patched)
> > 
> >
> > Could you please add assert message.

It's not part of the changed code. `TestV2JobServlet` deserves refactoring - in 
a separate JIRA.


> On Nov. 30, 2018, 1:01 p.m., Andras Salamon wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Lines 127 (patched)
> > 
> >
> > Could you please assertMessage

Done.


> On Nov. 30, 2018, 1:01 p.m., Andras Salamon wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Lines 179-184 (original), 183-188 (patched)
> > 
> >
> > This code (PUT) appears multiple times. Could you extract it to a 
> > method?

It's not part of the changed code. `TestV2JobServlet` deserves refactoring - in 
a separate JIRA.


> On Nov. 30, 2018, 1:01 p.m., Andras Salamon wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Line 184 (original), 188 (patched)
> > 
> >
> > Could you please add assert message.

It's not part of the changed code. `TestV2JobServlet` deserves refactoring - in 
a separate JIRA.


> On Nov. 30, 2018, 1:01 p.m., Andras Salamon wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Lines 218-223 (original), 222-227 (patched)
> > 
> >
> > This code (PUT) appears multiple times. Could you extract it to a 
> > method?

It's not part of the changed code. `TestV2JobServlet` deserves refactoring - in 
a separate JIRA.


> On Nov. 30, 2018, 1:01 p.m., Andras Salamon wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Line 223 (original), 227 (patched)
> > 
> >
> > Could you please add assert message.

It's not part of the changed code. `TestV2JobServlet` deserves refactoring - in 
a separate JIRA.


> On Nov. 30, 2018, 1:01 p.m., Andras Salamon wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Lines 313-316 (original), 317-320 (patched)
> > 
> >
> > This code (GET) appears multiple times. Could you extract it to a 
> > method?

It's not part of the changed code. `TestV2JobServlet` deserves refactoring - in 
a separate JIRA.


> On Nov. 30, 2018, 1:01 p.m., Andras Salamon wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Lines 334-337 (original), 338-341 (patched)
> > 
> >
> > This code (GET) appears multiple times. Could you extract it to a 
> > method?

It's not part of the changed code. `TestV2JobServlet` deserves refactoring - in 
a separate JIRA.


> On Nov. 30, 2018, 1:01 p.m., Andras Salamon wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Lines 355-358 (original), 359-362 (patched)
> > 
> >
> > This code (GET) appears multiple times. Could you extract it to a 
> > method?

It's not part of the changed code. `TestV2JobServlet` deserves refactoring - in 
a separate JIRA.


> On Nov. 30, 2018, 1:01 p.m., Andras Salamon wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Lines 419-422 (original), 423-426 (patched)
> > 
> >
> > This code (GET) appears multiple times. Could you extract it to a 
> > method?

It's not part of the changed code. `TestV2JobServlet` deserves refactoring - in 
a separate JIRA.


> On Nov. 30, 2018, 1:01 p.m., Andras Salamon wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Lines 440-443 (original), 444-447 (patched)
> > 
> >
> > This code (GET) appears multiple times. Could you extract it to a 
> > method?

It's not part of the changed code. `TestV2JobServlet` deserves refactoring - in 
a separate JIRA.


- András


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


On Nov. 30, 2018, 12:49 p.m., András Piros wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> 

Re: Review Request 69408: OOZIE-3382 - Optimize SshActionExecutor's drainBuffers method

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Nov. 30, 2018, 4:04 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69408/
> ---
> 
> (Updated Nov. 30, 2018, 4:04 p.m.)
> 
> 
> Review request for oozie, András Piros and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3382 - Optimize SshActionExecutor's drainBuffers method
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/action/ssh/SshActionExecutor.java 
> 3e0e3c573 
>   core/src/main/java/org/apache/oozie/util/BufferDrainer.java PRE-CREATION 
>   core/src/test/java/org/apache/oozie/util/BlockingInputStream.java 
> PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/util/BlockingWritesExitValueProcess.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/oozie/util/DrainerTestCase.java PRE-CREATION 
>   core/src/test/java/org/apache/oozie/util/TestBlockingInputStream.java 
> PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/util/TestBlockingWritesExitValueProcess.java
>  PRE-CREATION 
>   core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69408/diff/4/
> 
> 
> Testing
> ---
> 
> Unit tests executed 50 times using grind.
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Review Request 69492: OOZIE-3389 Getting input dependency list on the UI throws NPE

[András Piros via Review Board]

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

Review request for oozie, Andras Salamon and Kinga Marton.


Repository: oozie-git


Description
---

OOZIE-3389 Getting input dependency list on the UI throws NPE


Diffs
-

  
core/src/main/java/org/apache/oozie/command/coord/CoordActionMissingDependenciesXCommand.java
 d37cfe5125a5c3a665b3e33e6ac3fd5581fe278f 
  
core/src/main/java/org/apache/oozie/coord/input/dependency/CoordOldInputDependency.java
 56aef1c1779ed7446852af42f25c5ca058e473e4 
  
core/src/test/java/org/apache/oozie/coord/input/dependency/TestCoordOldInputDependency.java
 PRE-CREATION 
  core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java 
bacfe89bb3d836da1f6e559fa68a5bf4db970b09 


Diff: https://reviews.apache.org/r/69492/diff/1/


Testing
---

Introduced new unit test class `TestCoordOldInputDependency`.


Thanks,

András Piros

Re: Review Request 69469: OOZIE-3393: Add Oozie instrumentation delayed metric in CoordMaterializeTriggerService

[András Piros via Review Board]

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



Overall direction is OK. Please add some unit tests, and sample 
`oozie-instrumentation.log` snippets.


core/src/main/java/org/apache/oozie/service/CoordMaterializeTriggerService.java
Lines 71 (patched)


`currentMaterializedJobsCount` would be a better name.



core/src/main/java/org/apache/oozie/service/CoordMaterializeTriggerService.java
Lines 72 (patched)


`currentMaterializedDelayedJobsCount` would be a better name.



core/src/main/java/org/apache/oozie/service/CoordMaterializeTriggerService.java
Lines 174 (patched)


Typo in variable name. What about instead: `final long now = ...`


- András Piros


On Nov. 28, 2018, 11:56 a.m., zhang junfan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69469/
> ---
> 
> (Updated Nov. 28, 2018, 11:56 a.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> We have multiple Oozie scheduled clusters. The problem that is currently 
> encountered is that some materialize queues are too late to process during 
> the peak hours of the scheduled tasks. We need to get the metrics of the 
> delay queue for alarm monitoring and adjust the configuration to optimize.
> Currently we have added metrics for queue size and latency in Oozie 
> CoordMaterializeTriggerService class, in order to reflect the general trend 
> of current scheduling.
> 
> 
> Diffs
> -
> 
>   
> core/src/main/java/org/apache/oozie/service/CoordMaterializeTriggerService.java
>  1cbd4749 
> 
> 
> Diff: https://reviews.apache.org/r/69469/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> zhang junfan
> 
>

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[András Piros via Review Board]

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




client/src/main/java/org/apache/oozie/client/AuthOozieClient.java
Lines 105 (patched)


Are you sure you only get characters that fit into a file name on Linux 
OSes? I think special behavior is needed to mask such incorrect characters.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 199-201 (original), 243 (patched)


Deleting and assertion for non-existence missing. Is it by chance?



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 320 (patched)


A couple of other test scenarios would be also of importance:

* multiple `OozieCLI` calls to the same Oozie host w/ same context roots
* multiple `OozieCLI` calls to the same Oozie host w/ different context 
roots
* multiple `OozieCLI` calls to different Oozie host w/ and different 
context roots



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 322-338 (patched)


Please extract to a well-named method for clarity.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 328 (patched)


What about `try-with-resources` and omitting the `close()` call?



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 339 (patched)


Do we have to set this also when 
`oozie.authentication.signature.secret.file` has been set in beforehand?



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 343-350 (patched)


Extract method.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 347 (patched)


Please put appropriate assertion message.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 351-358 (patched)


Extract method.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 355 (patched)


Please put appropriate assertion message.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 366 (patched)


Assertion message.



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 368 (patched)


Assertion message.


- András Piros


On Nov. 28, 2018, 12:15 p.m., zhang junfan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69330/
> ---
> 
> (Updated Nov. 28, 2018, 12:15 p.m.)
> 
> 
> Review request for oozie, András Piros and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Auth token cache file name should include Oozie URL, link to oozie-3379.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
>   core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
> fc9d840b 
>   
> core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
>  04fde730 
> 
> 
> Diff: https://reviews.apache.org/r/69330/diff/3/
> 
> 
> Testing
> ---
> 
> I added a test for authOozieClients with the different oozieUrl.
> I changed the code for EmbeddedServletContainer. Because this patch needs to 
> bind the cache file to oozieUrl, you need to test whether multiple different 
> clients can use the cache. In the original EmbeddedServletContainer code, the 
> port was random and could not get a given container context path. So the port 
> binding needs to be added.
> 
> 
> File Attachments
> 
> 
> oozie-3379-6.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/11/27/ec465408-bcbc-44bc-a816-7ff73e3c0e68__oozie-3379-6.patch
> 
> 
> Thanks,
> 
> zhang junfan
> 
>

Re: Review Request 69408: OOZIE-3382 - Optimize SshActionExecutor's drainBuffers method

[András Piros via Review Board]

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



Thanks for the works, it's a very neat improvement to SSH action.

In general, a nice approach and pretty good coverage. Most of my comments point 
towards better readability / maintainability.

I'd extend the test coverage by adding:

* slow processes that drain up till several seconds
* processes that don't drain on a linear scale, but after a while streams get 
paused and then resumed w/ random pause times
* drain a couple of processes at a time in different threads, and see whether 
all of those finish correctly
* randomly failing while draining some parallel processes


core/src/main/java/org/apache/oozie/util/BufferDrainer.java
Lines 44-45 (patched)


A few thoughts on parameters etc.:

* `inputBuffer` and `errorBuffer` should not be inout parameters, but 
instance variables. There should be a `getInputBuffer()` / `getErrorBuffer()` 
methods that throw `IllegalStateException` when `drainBuffers()` hasn't yet 
been finished (not called or running)
* `p` and `maxLength` could be instance variables as well, initialized as 
constructor parameters
* this method should not be `static`



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


Dead code



core/src/main/java/org/apache/oozie/util/BufferDrainer.java
Lines 71 (patched)


Dead code



core/src/main/java/org/apache/oozie/util/BufferDrainer.java
Lines 82 (patched)


Maybe a `LOG.trace()` for better traceability in problematic cases.



core/src/main/java/org/apache/oozie/util/BufferDrainer.java
Lines 123-125 (patched)


Nice catch :)



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 49 (patched)


`EXIT_VALUE_ON_ERROR`



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 50-51 (patched)


`@Rule public ExpectedException expectedException = 
ExpectedException.none();` would be way more readable.



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 55 (patched)


``



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 58 (patched)


``



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 63 (patched)


Can it be a `static class`? I'd also make visibility to package private, 
and separately test logic of this class.

Would rather give a more descriptive name, like 
`BlockingWritesExitValueProcess`.



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 67 (patched)


Maybe better to extract to upper class level, as a `static class`, and test 
its functionality separately? And name it e.g. `BlockingInputStream`.



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 77-82 (patched)


Would extract to a method and do it on the constructor end.

Also unsure whether we should not write in the constructor but at the 
beginning at each `read()` call.



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 91 (patched)


Would extract to `final boolean someBytesRead` as per 
[Javadoc](https://docs.oracle.com/javase/7/docs/api/java/io/ByteArrayInputStream.html#read(byte[],%20int,%20int))



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 102-111 (patched)


`checkBlockedAndTryWriteNextChunk()`



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 117 (patched)


`tryWriteNextChunk()` might be a better name.



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 179-181 (patched)


Extract method `generateSamples()`.



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 194 (patched)


`testReadSinglePass()` would be a better name.



core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
Lines 195-196 (patched)


This code snippet appears multiple times. What if 

Re: Review Request 69330: OOZIE-3379 Auth token cache file name should include Oozie URL

[András Piros via Review Board]

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




client/src/main/java/org/apache/oozie/client/AuthOozieClient.java
Lines 73 (patched)


Can be `final`.



client/src/main/java/org/apache/oozie/client/AuthOozieClient.java
Lines 100 (patched)


Use `@VisibleForTesting`, and can be package-private.



client/src/main/java/org/apache/oozie/client/AuthOozieClient.java
Lines 101 (patched)


Why not use Guava's `BaseEncoding` like this?

```
BaseEncoding
.base64Url()
.omitPadding()
.encode("https://localhost:11443/oozie".getBytes(Charsets.UTF_8))
```



core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java
Lines 131-133 (patched)


Can you please extract to a well-named method, or leave a code comment for 
future maintainers, or both?



core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java
Line 128 (original), 136 (patched)


Can you please extract to a well-named method, or leave a code comment for 
future maintainers, or both?



core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
Lines 312-336 (patched)


Can you please:

* extract to separate test method
* provide assertion messages


- András Piros


On Nov. 14, 2018, 2:02 a.m., zhang junfan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69330/
> ---
> 
> (Updated Nov. 14, 2018, 2:02 a.m.)
> 
> 
> Review request for oozie, András Piros and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Auth token cache file name should include Oozie URL, link to oozie-3379.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 3a8b5ab6 
>   core/src/main/java/org/apache/oozie/test/EmbeddedServletContainer.java 
> fc9d840b 
>   
> core/src/test/java/org/apache/oozie/servlet/TestAuthFilterAuthOozieClient.java
>  04fde730 
> 
> 
> Diff: https://reviews.apache.org/r/69330/diff/1/
> 
> 
> Testing
> ---
> 
> I added a test for authOozieClients with the different oozieUrl.
> I changed the code for EmbeddedServletContainer. Because this patch needs to 
> bind the cache file to oozieUrl, you need to test whether multiple different 
> clients can use the cache. In the original EmbeddedServletContainer code, the 
> port was random and could not get a given container context path. So the port 
> binding needs to be added.
> 
> 
> Thanks,
> 
> zhang junfan
> 
>

Re: Review Request 69348: OOZIE-3381 [coordinator] Enhance logging of CoordElFunctions

[András Piros via Review Board]

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

(Updated Nov. 20, 2018, 10:54 a.m.)


Review request for oozie and Kinga Marton.


Changes
---

Update based on review comments.


Repository: oozie-git


Description
---

OOZIE-3381 [coordinator] Enhance logging of CoordElFunctions


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/command/XCommand.java 
a80444e14134ec4234e6edfef0888cf1e76566df 
  core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java 
10f4f0d8d14fbb8d60a2d09a45b8f3b3b089f461 
  core/src/test/java/org/apache/oozie/command/coord/CoordELExtensions.java 
796d19cfc9cbd03211746f9b94d0f67371249c7e 
  core/src/test/java/org/apache/oozie/coord/TestOozieTimeUnitConverter.java 
PRE-CREATION 


Diff: https://reviews.apache.org/r/69348/diff/2/

Changes: https://reviews.apache.org/r/69348/diff/1-2/


Testing
---

`TestCoordELFunctions`


Thanks,

András Piros

Review Request 69348: OOZIE-3381 [coordinator] Enhance logging of CoordElFunctions

[András Piros via Review Board]

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

Review request for oozie and Kinga Marton.


Repository: oozie-git


Description
---

OOZIE-3381 [coordinator] Enhance logging of CoordElFunctions


Diffs
-

  core/src/main/java/org/apache/oozie/command/XCommand.java 
a80444e14134ec4234e6edfef0888cf1e76566df 
  core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java 
10f4f0d8d14fbb8d60a2d09a45b8f3b3b089f461 


Diff: https://reviews.apache.org/r/69348/diff/1/


Testing
---

`TestCoordELFunctions`


Thanks,

András Piros

Re: Review Request 69252: OOZIE 3378 - Coordinator action's status is SUBMITTED after E1003 error

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Nov. 6, 2018, 9 a.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69252/
> ---
> 
> (Updated Nov. 6, 2018, 9 a.m.)
> 
> 
> Review request for oozie, András Piros, Peter Cseh, and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE 3378 - Coordinator action's status is SUBMITTED after E1003 error
> 
> 
> Diffs
> -
> 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordActionStartXCommand.java
>  267943ee5 
>   
> core/src/test/java/org/apache/oozie/command/coord/TestCoordActionStartXCommand.java
>  4215a32b1 
> 
> 
> Diff: https://reviews.apache.org/r/69252/diff/2/
> 
> 
> Testing
> ---
> 
> Created a coordinator.xml which gives us E1003 error. (See details in 
> upstream Jira).
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 69252: OOZIE 3378 - Coordinator action's status is SUBMITTED after E1003 error

[András Piros via Review Board]

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




core/src/test/java/org/apache/oozie/command/coord/TestCoordActionStartXCommand.java
Lines 238 (patched)


Isn't a `Map` better?


- András Piros


On Nov. 5, 2018, 12:58 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69252/
> ---
> 
> (Updated Nov. 5, 2018, 12:58 p.m.)
> 
> 
> Review request for oozie, András Piros, Peter Cseh, and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE 3378 - Coordinator action's status is SUBMITTED after E1003 error
> 
> 
> Diffs
> -
> 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordActionStartXCommand.java
>  267943ee5 
>   
> core/src/test/java/org/apache/oozie/command/coord/TestCoordActionStartXCommand.java
>  4215a32b1 
> 
> 
> Diff: https://reviews.apache.org/r/69252/diff/1/
> 
> 
> Testing
> ---
> 
> Created a coordinator.xml which gives us E1003 error. (See details in 
> upstream Jira).
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Review Request 69251: OOZIE-3377 [docs] Remaining 5.1.0 documentation changes

[András Piros via Review Board]

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

Review request for oozie, Peter Cseh and Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-3377 [docs] Remaining 5.1.0 documentation changes


Diffs
-

  docs/src/site/markdown/DG_GitActionExtension.md PRE-CREATION 
  docs/src/site/markdown/WorkflowFunctionalSpec.md 
463635bf3e60db975a51270a1bd7686033c328f0 
  docs/src/site/markdown/index.md 021622277f67281c396988a139ae4dc92e135bbe 


Diff: https://reviews.apache.org/r/69251/diff/1/


Testing
---

```
mvn clean install -DskipTests -Denforcer.skip=true -Dcheckstyle.skip=true 
-Dfindbugs.skip=true -PgenerateDocs
```


Thanks,

András Piros

Review Request 69250: OOZIE-3376 [tests] TestGraphGenerator should assume JDK8 minor version at least 1.8.0_u40

[András Piros via Review Board]

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

Review request for oozie, Peter Cseh and Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-3376 [tests] TestGraphGenerator should assume JDK8 minor version at least 
1.8.0_u40


Diffs
-

  core/src/main/resources/oozie-log4j.properties 
ba8d7b9eac64970b6ad33256eb32b4d2a2819f9f 
  core/src/test/java/org/apache/oozie/util/graph/TestGraphGenerator.java 
9b0e263bf2d9fb796bc26d50d16f72237c24baad 
  docs/src/site/markdown/WebServicesAPI.md 
7cf62e5416c8198f5d3871d0adb812a10fc02011 


Diff: https://reviews.apache.org/r/69250/diff/1/


Testing
---

`TestGraphGenerator` methods modified.


Thanks,

András Piros

Review Request 68909: amend OOZIE-3160 PriorityDelayQueue put()/take() can cause significant CPU load due to busy waiting

[András Piros via Review Board]

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

Review request for oozie and Peter Bacsko.


Repository: oozie-git


Description
---

amend OOZIE-3160 PriorityDelayQueue put()/take() can cause significant CPU load 
due to busy waiting


Diffs
-

  core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java 
b18a37a2e78321692f52e5e7933da418fbedc17d 
  core/src/main/java/org/apache/oozie/service/CallableQueueService.java 
dc9a0991e49557e42b40139e46ee7732a4cb4398 
  core/src/main/resources/oozie-default.xml 
f292ad3e09eb2ff3115e09670a8784064cd751c5 
  core/src/test/java/org/apache/oozie/service/TestAsyncXCommandExecutor.java 
2dce4094cf7008ff9d8225b62662c87db8d9c274 
  
core/src/test/java/org/apache/oozie/service/TestHAPartitionDependencyManagerService.java
 3e1df07517556b9efaf300916d4d5f717932c6f6 
  core/src/test/java/org/apache/oozie/test/XTestCase.java 
3048ddaabb909a3bf44caec9ea51926f9dcb0a94 
  core/src/test/java/org/apache/oozie/test/ZKXTestCase.java 
abc7c9fb9262646887e8a7cc6fcb9785d7a12c0d 


Diff: https://reviews.apache.org/r/68909/diff/1/


Testing
---


Thanks,

András Piros

Review Request 68837: OOZIE-3340 [fluent-job] Create error handler ACTION only if needed

[András Piros via Review Board]

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

Review request for oozie, Kinga Marton and Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-3340 [fluent-job] Create error handler ACTION only if needed


Diffs
-

  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/ShellAction.java
 c1b79aa90a0d4e3e456432fed256dc5c3c940eae 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GraphNodesToWORKFLOWAPPConverter.java
 da4691fbc170f09efbf7a93c85b177f8efa4aeb4 
  
fluent-job/fluent-job-api/src/test/java/org/apache/oozie/fluentjob/api/mapping/TestGraphMapping.java
 0563ec63b148ba993ae859b81ccfe129ca67df87 


Diff: https://reviews.apache.org/r/68837/diff/1/


Testing
---

`TestGraphMapping#testMappingGraphFromWorkflow()` extended to cover.


Thanks,

András Piros

Re: Review Request 68737: OOZIE-3307 [core][oya] Limit heap usage of LauncherAM

[András Piros via Review Board]

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

(Updated Sept. 24, 2018, 3:17 p.m.)


Review request for oozie and Peter Bacsko.


Changes
---

Refactored code so that `BytesAndUOMConverter` is a separate class only being 
used by `LauncherMainTester`, and user configured heap modifier options are 
only checked for presence. When present, maximum heap option isn't set. Former 
`LastParameterValueExtractor` is deleted.


Repository: oozie-git


Description
---

OOZIE-3307 [core][oya] Limit heap usage of LauncherAM


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/BytesAndUOMConverter.java 
PRE-CREATION 
  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
0385c77b4c01a3ec7ee9147b2369db5b1548ad80 
  core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java 
ada7005e01c949a3800427678b95edf98953e635 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestBytesAndUOMConverter.java 
PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestHeapModifiersPattern.java 
PRE-CREATION 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
784dc96c314ca9cde3685b0d8360f9b0d73b77cb 


Diff: https://reviews.apache.org/r/68737/diff/6/

Changes: https://reviews.apache.org/r/68737/diff/5-6/


Testing
---

New test cases / classes added:

* `TestJavaActionExecutor`
* `TestBytesAndUOMConverter`
* `TestLastParameterValueExtractor`


Thanks,

András Piros

Re: Review Request 68737: OOZIE-3307 [core][oya] Limit heap usage of LauncherAM

[András Piros via Review Board]

> On Sept. 20, 2018, 3:05 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
> > Lines 2063 (patched)
> > 
> >
> > General thoughts, as we discussed:
> > 
> > 1. Code should not throw an exception when the numbers cannot be parsed
> > 2. -Xms must be considered as well. If Xmx < Xms, Hotspot will exit 
> > with an error message.
> > 
> > In general, we might better off just check the presence of -Xmx and 
> > -Xms and just simply print a general warning to the user, pointing out how 
> > heap settings might affect YARN.
> > 
> > If it's too much code to parse/calculate simple things, just leave 
> > these alone.

Refactored code so that `BytesAndUOMConverter` is a separate class only being 
used by `LauncherMainTester`, and user configured heap modifier options are 
only checked for presence. When present, maximum heap option isn't set. Former 
`LastParameterValueExtractor` is deleted.


- András


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


On Sept. 20, 2018, 2:27 p.m., András Piros wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68737/
> ---
> 
> (Updated Sept. 20, 2018, 2:27 p.m.)
> 
> 
> Review request for oozie and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3307 [core][oya] Limit heap usage of LauncherAM
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
> 0385c77b4c01a3ec7ee9147b2369db5b1548ad80 
>   core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java 
> ada7005e01c949a3800427678b95edf98953e635 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestBytesAndUOMConverter.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
> 784dc96c314ca9cde3685b0d8360f9b0d73b77cb 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestLastParameterValueExtractor.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68737/diff/5/
> 
> 
> Testing
> ---
> 
> New test cases / classes added:
> 
> * `TestJavaActionExecutor`
> * `TestBytesAndUOMConverter`
> * `TestLastParameterValueExtractor`
> 
> 
> Thanks,
> 
> András Piros
> 
>

Re: Review Request 68783: OOZIE-3342: Missing examples archive from fluentjob examples

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Sept. 20, 2018, 1:53 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68783/
> ---
> 
> (Updated Sept. 20, 2018, 1:53 p.m.)
> 
> 
> Review request for oozie, András Piros and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3342: Missing examples archive from fluentjob examples
> 
> 
> Diffs
> -
> 
>   docs/src/site/markdown/DG_FluentJobAPI.md bd3651701 
>   examples/src/main/java/org/apache/oozie/example/fluentjob/JavaMain.java 
> a8948fd13 
>   examples/src/main/java/org/apache/oozie/example/fluentjob/Spark.java 
> 952733622 
> 
> 
> Diff: https://reviews.apache.org/r/68783/diff/1/
> 
> 
> Testing
> ---
> 
> I've also documented the JavaMain example execution.
> 
> To compile the examples we also need oozie-fluent-job-api jar file. I'm not 
> sure if we package it right know.
> 
> My very own `./distro/target/oozie-5.1.0-SNAPSHOT-distro.tar.gz` file 
> contains the 
> `oozie-5.1.0-SNAPSHOT/embedded-oozie-server/webapp/WEB-INF/lib/oozie-fluent-job-api-5.1.0-SNAPSHOT.jar`
>  file. This is not a nice place to store this file, but it can be used to 
> compile the examples.
> 
> On the other hand, if I create a `oozie-5.1.0-SNAPSHOT.tar.gz` file using 
> `bin/create-release-artifact` I cannot find the `oozie-fluent-job-api*.jar` 
> file in it.
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 68737: OOZIE-3307 [core][oya] Limit heap usage of LauncherAM

[András Piros via Review Board]

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

(Updated Sept. 20, 2018, 2:27 p.m.)


Review request for oozie and Peter Bacsko.


Changes
---

Removed method `JavaActionExecutor#setOrRemoveConfiguredMaxHeap()` and changed 
behavior so that when there is a max heap configuration already set by the user 
this will take precedence over the calculated one.


Repository: oozie-git


Description
---

OOZIE-3307 [core][oya] Limit heap usage of LauncherAM


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
0385c77b4c01a3ec7ee9147b2369db5b1548ad80 
  core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java 
ada7005e01c949a3800427678b95edf98953e635 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestBytesAndUOMConverter.java 
PRE-CREATION 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
784dc96c314ca9cde3685b0d8360f9b0d73b77cb 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestLastParameterValueExtractor.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/68737/diff/5/

Changes: https://reviews.apache.org/r/68737/diff/4-5/


Testing
---

New test cases / classes added:

* `TestJavaActionExecutor`
* `TestBytesAndUOMConverter`
* `TestLastParameterValueExtractor`


Thanks,

András Piros

Re: Review Request 68140: OOZIE-3229 Improved filtering options in V2SLAServlet

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Sept. 19, 2018, 4:07 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68140/
> ---
> 
> (Updated Sept. 19, 2018, 4:07 p.m.)
> 
> 
> Review request for oozie, András Piros and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Added lots of new filter fields, also refactored 
> SLASummaryGetForFilterJPAExecutor class to eliminate FindBugs errors. I'm not 
> sure about the filter field names.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/OozieClient.java 949b4532f 
>   client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 
> 9873ff3ad 
>   core/src/main/java/org/apache/oozie/FilterParser.java PRE-CREATION 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
>  b54161e98 
>   core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java 3982d1e06 
>   core/src/test/java/org/apache/oozie/TestFilterParser.java PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java
>  PRE-CREATION 
>   core/src/test/java/org/apache/oozie/servlet/TestV2SLAServlet.java aa633225b 
>   core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java 
> PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletIntegration.java 
> PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletSLAJSONResponse.java
>  PRE-CREATION 
>   core/src/test/java/org/apache/oozie/servlet/V2SLAServletTestCase.java 
> PRE-CREATION 
>   docs/src/site/markdown/DG_SLAMonitoring.md 0831b93bd 
>   webapp/src/main/webapp/console/sla/css/oozie-sla.css d2f2deeec 
>   webapp/src/main/webapp/console/sla/js/oozie-sla.js 2ecad228a 
>   webapp/src/main/webapp/console/sla/oozie-sla.html e5bf6275a 
> 
> 
> Diff: https://reviews.apache.org/r/68140/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 68737: OOZIE-3307 [core][oya] Limit heap usage of LauncherAM

[András Piros via Review Board]

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

(Updated Sept. 19, 2018, 10:38 a.m.)


Review request for oozie and Peter Bacsko.


Changes
---

Addressing review comments.


Repository: oozie-git


Description
---

OOZIE-3307 [core][oya] Limit heap usage of LauncherAM


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
0385c77b4c01a3ec7ee9147b2369db5b1548ad80 
  core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java 
ada7005e01c949a3800427678b95edf98953e635 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestBytesAndUOMConverter.java 
PRE-CREATION 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
784dc96c314ca9cde3685b0d8360f9b0d73b77cb 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestLastParameterValueExtractor.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/68737/diff/4/

Changes: https://reviews.apache.org/r/68737/diff/3-4/


Testing
---

New test cases / classes added:

* `TestJavaActionExecutor`
* `TestBytesAndUOMConverter`
* `TestLastParameterValueExtractor`


Thanks,

András Piros

Re: Review Request 68737: OOZIE-3307 [core][oya] Limit heap usage of LauncherAM

[András Piros via Review Board]

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

(Updated Sept. 18, 2018, 1:06 p.m.)


Review request for oozie and Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-3307 [core][oya] Limit heap usage of LauncherAM


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
0385c77b4c01a3ec7ee9147b2369db5b1548ad80 
  core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java 
ada7005e01c949a3800427678b95edf98953e635 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestBytesAndUOMConverter.java 
PRE-CREATION 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
784dc96c314ca9cde3685b0d8360f9b0d73b77cb 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestLastParameterValueExtractor.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/68737/diff/3/

Changes: https://reviews.apache.org/r/68737/diff/2-3/


Testing
---

New test cases / classes added:

* `TestJavaActionExecutor`
* `TestBytesAndUOMConverter`
* `TestLastParameterValueExtractor`


Thanks,

András Piros

Re: Review Request 68140: OOZIE-3229 Improved filtering options in V2SLAServlet

[András Piros via Review Board]

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



Great work thus far! Only a couple of thingies left.


core/src/main/java/org/apache/oozie/FilterParser.java
Lines 44-45 (patched)


What about using Guava's 
[SortedSetMultimap](https://google.github.io/guava/releases/21.0/api/docs/com/google/common/collect/SortedSetMultimap.html)
 instead?



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 101-108 (patched)


Would need a short example of a bundle ID here, to better see what's this 
regexp all about.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 68-75 (original), 123-130 (patched)


Any ideas of better names here? The format `*START_START` and `*END_END` 
isn't easily understandable.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Line 115 (original), 202 (patched)


Should be `static class` if possible.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Line 116 (original), 203 (patched)


`LinkedHashMap` would be a better choice, for maintaining the insertion 
order.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 268 (patched)


Why not use `SLASummaryGetForFilterJPAExecutor.class.getSimpleName()` 
instead?



core/src/test/java/org/apache/oozie/TestFilterParser.java
Lines 32 (patched)


Why not use `@Test` annotations and not `extends TestCase` instead?



core/src/test/java/org/apache/oozie/TestFilterParser.java
Lines 83-86 (patched)


This stuff would be also simpler using Guava's `OrderedSetMultimap`.



core/src/test/java/org/apache/oozie/TestFilterParser.java
Lines 91-96 (patched)


This stuff would be also simpler using Guava's `OrderedSetMultimap`.



core/src/test/java/org/apache/oozie/TestFilterParser.java
Lines 101-104 (patched)


This stuff would be also simpler using Guava's `OrderedSetMultimap`.



core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java
Lines 58-60 (patched)


Is it still needed as we use `ExpectedException`?



core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java
Lines 69-71 (patched)


Is it still needed as we use `ExpectedException`?



core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java
Lines 80-82 (patched)


Is it still needed as we use `ExpectedException`?



core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java
Lines 90-92 (patched)


Is it still needed as we use `ExpectedException`?



core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java
Lines 136-138 (patched)


Is it still needed as we use `ExpectedException`?



core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java
Lines 147-149 (patched)


Is it still needed as we use `ExpectedException`?



core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java
Lines 193-195 (patched)


Is it still needed as we use `ExpectedException`?



core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java
Lines 207-209 (patched)


Is it still needed as we use `ExpectedException`?



core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java
Lines 221-223 (patched)


Is it still needed as we use `ExpectedException`?

Re: Review Request 68737: OOZIE-3307 [core][oya] Limit heap usage of LauncherAM

[András Piros via Review Board]

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

(Updated Sept. 18, 2018, 8:20 a.m.)


Review request for oozie and Peter Bacsko.


Changes
---

Addressed pre-commit issues:

* `FINDBUGS_DIFF` error because of complex regexp
* `RAT` errors on `HEAD` because of incorrect settings


Repository: oozie-git


Description
---

OOZIE-3307 [core][oya] Limit heap usage of LauncherAM


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
0385c77b4c01a3ec7ee9147b2369db5b1548ad80 
  core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java 
ada7005e01c949a3800427678b95edf98953e635 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestBytesAndUOMConverter.java 
PRE-CREATION 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
784dc96c314ca9cde3685b0d8360f9b0d73b77cb 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestLastParameterValueExtractor.java
 PRE-CREATION 
  pom.xml d6440e44315c6bb114ac57ebd7081511fa5faf43 


Diff: https://reviews.apache.org/r/68737/diff/2/

Changes: https://reviews.apache.org/r/68737/diff/1-2/


Testing
---

New test cases / classes added:

* `TestJavaActionExecutor`
* `TestBytesAndUOMConverter`
* `TestLastParameterValueExtractor`


Thanks,

András Piros

Review Request 68737: OOZIE-3307 [core][oya] Limit heap usage of LauncherAM

[András Piros via Review Board]

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

Review request for oozie and Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-3307 [core][oya] Limit heap usage of LauncherAM


Diffs
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
0385c77b4c01a3ec7ee9147b2369db5b1548ad80 
  core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java 
ada7005e01c949a3800427678b95edf98953e635 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestBytesAndUOMConverter.java 
PRE-CREATION 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
784dc96c314ca9cde3685b0d8360f9b0d73b77cb 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestLastParameterValueExtractor.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/68737/diff/1/


Testing
---

New test cases / classes added:

* `TestJavaActionExecutor`
* `TestBytesAndUOMConverter`
* `TestLastParameterValueExtractor`


Thanks,

András Piros

Re: Review Request 68628: OOZIE-2734 - Switch docs from twiki to markdown

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Sept. 13, 2018, 12:34 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68628/
> ---
> 
> (Updated Sept. 13, 2018, 12:34 p.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2734 - Switch docs from twiki to markdown
> 
> 
> Diffs
> -
> 
>   README.txt 8cc0f59ef 
>   docs/pom.xml ae2e94c9c 
>   docs/src/site/markdown/AG_OozieLogging.md PRE-CREATION 
>   docs/src/site/markdown/DG_QuickStart.md PRE-CREATION 
>   docs/src/site/markdown/ENG_Building.md PRE-CREATION 
>   docs/src/site/markdown/ENG_Custom_Authentication.md PRE-CREATION 
>   docs/src/site/markdown/index.md PRE-CREATION 
>   docs/src/site/twiki/AG_ActionConfiguration.twiki 8c032a754 
>   docs/src/site/twiki/AG_HadoopConfiguration.twiki 528bf4a41 
>   docs/src/site/twiki/AG_Install.twiki b8031c82e 
>   docs/src/site/twiki/AG_Monitoring.twiki 425f55435 
>   docs/src/site/twiki/AG_OozieLogging.twiki ecdcfd33e 
>   docs/src/site/twiki/AG_OozieUpgrade.twiki 302406482 
>   docs/src/site/twiki/BundleFunctionalSpec.twiki 9749df597 
>   docs/src/site/twiki/CoordinatorFunctionalSpec.twiki cd416d4cd 
>   docs/src/site/twiki/DG_ActionAuthentication.twiki bbf2d57d3 
>   docs/src/site/twiki/DG_CommandLineTool.twiki 8d33c508e 
>   docs/src/site/twiki/DG_CoordinatorRerun.twiki fbb1376d5 
>   docs/src/site/twiki/DG_CustomActionExecutor.twiki 783148495 
>   docs/src/site/twiki/DG_DistCpActionExtension.twiki 8bab3da44 
>   docs/src/site/twiki/DG_EmailActionExtension.twiki 4de290ccf 
>   docs/src/site/twiki/DG_Examples.twiki 5323a1773 
>   docs/src/site/twiki/DG_FluentJobAPI.twiki c8b764bb0 
>   docs/src/site/twiki/DG_HCatalogIntegration.twiki d3107b453 
>   docs/src/site/twiki/DG_Hive2ActionExtension.twiki efbe56dab 
>   docs/src/site/twiki/DG_HiveActionExtension.twiki aaa74fa7b 
>   docs/src/site/twiki/DG_JMSNotifications.twiki a4b0f0d1a 
>   docs/src/site/twiki/DG_Overview.twiki 3ec94a2f1 
>   docs/src/site/twiki/DG_QuickStart.twiki d6a0069ac 
>   docs/src/site/twiki/DG_SLAMonitoring.twiki c91c227dc 
>   docs/src/site/twiki/DG_ShellActionExtension.twiki 5894c280a 
>   docs/src/site/twiki/DG_SparkActionExtension.twiki ce80e45fc 
>   docs/src/site/twiki/DG_SqoopActionExtension.twiki 0317d0cec 
>   docs/src/site/twiki/DG_SshActionExtension.twiki 5a51d49c5 
>   docs/src/site/twiki/DG_WorkflowReRun.twiki 88d982b75 
>   docs/src/site/twiki/ENG_Building.twiki b86102683 
>   docs/src/site/twiki/ENG_Custom_Authentication.twiki 3b8202d0d 
>   docs/src/site/twiki/ENG_MiniOozie.twiki 0b1628986 
>   docs/src/site/twiki/WebServicesAPI.twiki f9008a60b 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 46a454c3a 
>   docs/src/site/twiki/index.twiki 3003fa992 
>   pom.xml 423d19d97 
> 
> 
> Diff: https://reviews.apache.org/r/68628/diff/3/
> 
> 
> Testing
> ---
> 
> HTML generated
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 68628: OOZIE-2734 - Switch docs from twiki to markdown

[András Piros via Review Board]

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




docs/src/site/markdown/ENG_Building.md
Lines 154 (patched)


html: URL not needed



docs/src/site/twiki/DG_Hive2ActionExtension.twiki
Line 30 (original), 30 (patched)


html, Markdown: url is not rendered correctly



docs/src/site/twiki/DG_Hive2ActionExtension.twiki
Line 149 (original), 151 (patched)


html, Markdown: url target page is missing, please remove



docs/src/site/twiki/DG_SLAMonitoring.twiki
Lines 415-416 (original), 444-445 (patched)


html



docs/src/site/twiki/DG_ShellActionExtension.twiki
Lines 121-122 (original), 122-123 (patched)


html



docs/src/site/twiki/DG_SparkActionExtension.twiki
Lines 231-232 (original), 238-239 (patched)


html, Markdown: link not rendered correctly



docs/src/site/twiki/WebServicesAPI.twiki
Line 760 (original), 793 (patched)


Markdown: newline needed between `required` and `oozie.pig.script.params.n`



docs/src/site/twiki/WebServicesAPI.twiki
Lines 1433 (patched)


Markdown: remove `` within `scope=\`



docs/src/site/twiki/WebServicesAPI.twiki
Lines 1632 (patched)


html


- András Piros


On Sept. 12, 2018, 8:50 a.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68628/
> ---
> 
> (Updated Sept. 12, 2018, 8:50 a.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2734 - Switch docs from twiki to markdown
> 
> 
> Diffs
> -
> 
>   README.txt 8cc0f59ef 
>   docs/pom.xml ae2e94c9c 
>   docs/src/site/markdown/AG_OozieLogging.md PRE-CREATION 
>   docs/src/site/markdown/DG_QuickStart.md PRE-CREATION 
>   docs/src/site/markdown/ENG_Building.md PRE-CREATION 
>   docs/src/site/markdown/ENG_Custom_Authentication.md PRE-CREATION 
>   docs/src/site/markdown/index.md PRE-CREATION 
>   docs/src/site/twiki/AG_ActionConfiguration.twiki 8c032a754 
>   docs/src/site/twiki/AG_HadoopConfiguration.twiki 528bf4a41 
>   docs/src/site/twiki/AG_Install.twiki b8031c82e 
>   docs/src/site/twiki/AG_Monitoring.twiki 425f55435 
>   docs/src/site/twiki/AG_OozieLogging.twiki ecdcfd33e 
>   docs/src/site/twiki/AG_OozieUpgrade.twiki 302406482 
>   docs/src/site/twiki/BundleFunctionalSpec.twiki 9749df597 
>   docs/src/site/twiki/CoordinatorFunctionalSpec.twiki cd416d4cd 
>   docs/src/site/twiki/DG_ActionAuthentication.twiki bbf2d57d3 
>   docs/src/site/twiki/DG_CommandLineTool.twiki 8d33c508e 
>   docs/src/site/twiki/DG_CoordinatorRerun.twiki fbb1376d5 
>   docs/src/site/twiki/DG_CustomActionExecutor.twiki 783148495 
>   docs/src/site/twiki/DG_DistCpActionExtension.twiki 8bab3da44 
>   docs/src/site/twiki/DG_EmailActionExtension.twiki 4de290ccf 
>   docs/src/site/twiki/DG_Examples.twiki 5323a1773 
>   docs/src/site/twiki/DG_FluentJobAPI.twiki c8b764bb0 
>   docs/src/site/twiki/DG_HCatalogIntegration.twiki d3107b453 
>   docs/src/site/twiki/DG_Hive2ActionExtension.twiki efbe56dab 
>   docs/src/site/twiki/DG_HiveActionExtension.twiki aaa74fa7b 
>   docs/src/site/twiki/DG_JMSNotifications.twiki a4b0f0d1a 
>   docs/src/site/twiki/DG_Overview.twiki 3ec94a2f1 
>   docs/src/site/twiki/DG_QuickStart.twiki d6a0069ac 
>   docs/src/site/twiki/DG_SLAMonitoring.twiki c91c227dc 
>   docs/src/site/twiki/DG_ShellActionExtension.twiki 5894c280a 
>   docs/src/site/twiki/DG_SparkActionExtension.twiki ce80e45fc 
>   docs/src/site/twiki/DG_SqoopActionExtension.twiki 0317d0cec 
>   docs/src/site/twiki/DG_SshActionExtension.twiki 5a51d49c5 
>   docs/src/site/twiki/DG_WorkflowReRun.twiki 88d982b75 
>   docs/src/site/twiki/ENG_Building.twiki b86102683 
>   docs/src/site/twiki/ENG_Custom_Authentication.twiki 3b8202d0d 
>   docs/src/site/twiki/ENG_MiniOozie.twiki 0b1628986 
>   docs/src/site/twiki/WebServicesAPI.twiki f9008a60b 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 46a454c3a 
>   docs/src/site/twiki/index.twiki 3003fa992 
>   pom.xml 423d19d97 
> 
> 
> Diff: https://reviews.apache.org/r/68628/diff/2/
> 
> 
> Testing
> ---
> 
> HTML generated
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 68628: OOZIE-2734 - Switch docs from twiki to markdown

[András Piros via Review Board]

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




docs/src/site/twiki/DG_CommandLineTool.twiki
Lines 461-469 (original), 482-490 (patched)


html



docs/src/site/twiki/DG_CommandLineTool.twiki
Lines 755-761 (original), 789-795 (patched)


html



docs/src/site/twiki/DG_CommandLineTool.twiki
Lines 1102-1104 (original), 1148-1150 (patched)


html



docs/src/site/twiki/DG_CommandLineTool.twiki
Lines 1144-1150 (original), 1192-1198 (patched)


html



docs/src/site/twiki/DG_CommandLineTool.twiki
Lines 1188-1194 (original), 1238-1244 (patched)


html



docs/src/site/twiki/DG_CommandLineTool.twiki
Line 1251 (original), 1304 (patched)


html



docs/src/site/twiki/DG_CommandLineTool.twiki
Line 1347 (original), 1403 (patched)


html



docs/src/site/twiki/DG_CommandLineTool.twiki
Lines 1723-1725 (original), 1793-1795 (patched)


html



docs/src/site/twiki/DG_CommandLineTool.twiki
Line 1967 (original), 2047 (patched)


html



docs/src/site/twiki/DG_Examples.twiki
Line 134 (original), 139 (patched)


html



docs/src/site/twiki/DG_FluentJobAPI.twiki
Line 178 (original), 180 (patched)


html



docs/src/site/twiki/DG_HCatalogIntegration.twiki
Line 10 (original), 10 (patched)


html



docs/src/site/twiki/DG_HCatalogIntegration.twiki
Line 14 (original), 14 (patched)


html



docs/src/site/twiki/DG_ShellActionExtension.twiki
Lines 224-226 (original), 229-231 (patched)


html


- András Piros


On Sept. 5, 2018, 10 a.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68628/
> ---
> 
> (Updated Sept. 5, 2018, 10 a.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2734 - Switch docs from twiki to markdown
> 
> 
> Diffs
> -
> 
>   docs/pom.xml ae2e94c9c 
>   docs/src/site/markdown/AG_OozieLogging.md PRE-CREATION 
>   docs/src/site/markdown/ENG_Building.md PRE-CREATION 
>   docs/src/site/markdown/index.md PRE-CREATION 
>   docs/src/site/twiki/AG_ActionConfiguration.twiki 8c032a754 
>   docs/src/site/twiki/AG_HadoopConfiguration.twiki 528bf4a41 
>   docs/src/site/twiki/AG_Install.twiki b8031c82e 
>   docs/src/site/twiki/AG_Monitoring.twiki 425f55435 
>   docs/src/site/twiki/AG_OozieLogging.twiki ecdcfd33e 
>   docs/src/site/twiki/AG_OozieUpgrade.twiki 302406482 
>   docs/src/site/twiki/BundleFunctionalSpec.twiki 9749df597 
>   docs/src/site/twiki/CoordinatorFunctionalSpec.twiki cd416d4cd 
>   docs/src/site/twiki/DG_ActionAuthentication.twiki bbf2d57d3 
>   docs/src/site/twiki/DG_CommandLineTool.twiki 8d33c508e 
>   docs/src/site/twiki/DG_CoordinatorRerun.twiki fbb1376d5 
>   docs/src/site/twiki/DG_CustomActionExecutor.twiki 783148495 
>   docs/src/site/twiki/DG_DistCpActionExtension.twiki 8bab3da44 
>   docs/src/site/twiki/DG_EmailActionExtension.twiki 4de290ccf 
>   docs/src/site/twiki/DG_Examples.twiki 5323a1773 
>   docs/src/site/twiki/DG_FluentJobAPI.twiki c8b764bb0 
>   docs/src/site/twiki/DG_HCatalogIntegration.twiki d3107b453 
>   docs/src/site/twiki/DG_Hive2ActionExtension.twiki efbe56dab 
>   docs/src/site/twiki/DG_HiveActionExtension.twiki aaa74fa7b 
>   docs/src/site/twiki/DG_JMSNotifications.twiki a4b0f0d1a 
>   docs/src/site/twiki/DG_Overview.twiki 3ec94a2f1 
>   docs/src/site/twiki/DG_QuickStart.twiki d6a0069ac 
>   docs/src/site/twiki/DG_SLAMonitoring.twiki c91c227dc 
>   docs/src/site/twiki/DG_ShellActionExtension.twiki 5894c280a 
>   docs/src/site/twiki/DG_SparkActionExtension.twiki ce80e45fc 
>   docs/src/site/twiki/DG_SqoopActionExtension.twiki 0317d0cec 
>   docs/src/site/twiki/DG_SshActionExtension.twiki 5a51d49c5 
>   docs/src/site/twiki/DG_WorkflowReRun.twiki 88d982b75 
>   docs/src/site/twiki/ENG_Building.twiki b86102683 
>   docs/src/site/twiki/ENG_Custom_Authentication.twiki 3b8202d0d 
>   docs/src/site/twiki/ENG_MiniOozie.twiki 0b1628986 
>   docs/src/site/twiki/WebServicesAPI.twiki f9008a60b 

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

(Updated Sept. 10, 2018, 3:37 p.m.)


Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
Peter Bacsko.


Changes
---

Addressing last FindBugs error.


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
05fac3953eed5a5e2a16fa362656596f3a962a88 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMain.java 
c9e2a914a4b88640ac1c370b8971355dc087a235 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/11/

Changes: https://reviews.apache.org/r/68237/diff/10-11/


Testing
---

Tested on a real cluster, plus added test cases to 
`TestMapReduceActionExecutor` and new test classes.


Thanks,

András Piros

Re: Review Request 68628: OOZIE-2734 - Switch docs from twiki to markdown

[András Piros via Review Board]

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



Reviewed following HTML / Markdown files:

 * `AG_ActionConfiguration`
 * `AG_HadoopConfiguration`
 * `AG_Install`
 * `AG_Monitoring`
 * `AG_OozieLogging`
 * `AG_OozieUpgrade`
 * `BundleFunctionalSpec`
 * `CoordinatorFunctionalSpec`


docs/src/site/twiki/AG_Install.twiki
Lines 243-245 (patched)


html



docs/src/site/twiki/BundleFunctionalSpec.twiki
Lines 93-101 (patched)


html



docs/src/site/twiki/CoordinatorFunctionalSpec.twiki
Lines 607-620 (original), 634-647 (patched)


html



docs/src/site/twiki/CoordinatorFunctionalSpec.twiki
Line 875 (original), 919 (patched)


html



docs/src/site/twiki/CoordinatorFunctionalSpec.twiki
Lines 890-900 (original), 934-944 (patched)


html



docs/src/site/twiki/CoordinatorFunctionalSpec.twiki
Lines 975-980 (original), 1019-1024 (patched)


html



docs/src/site/twiki/CoordinatorFunctionalSpec.twiki
Line 1009 (original), 1053 (patched)


html



docs/src/site/twiki/CoordinatorFunctionalSpec.twiki
Lines 1047-1075 (original), 1091-1119 (patched)


html



docs/src/site/twiki/CoordinatorFunctionalSpec.twiki
Lines 1886-1889 (original), 1956-1959 (patched)


html



docs/src/site/twiki/CoordinatorFunctionalSpec.twiki
Lines 3509-3511 (original), 3622-3624 (patched)


html



docs/src/site/twiki/CoordinatorFunctionalSpec.twiki
Lines 3514-3515 (original), 3627-3628 (patched)


html



docs/src/site/twiki/CoordinatorFunctionalSpec.twiki
Line 3609 (original), 3728 (patched)


html



docs/src/site/twiki/CoordinatorFunctionalSpec.twiki
Line 3630 (original), 3751 (patched)


html


- András Piros


On Sept. 5, 2018, 10 a.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68628/
> ---
> 
> (Updated Sept. 5, 2018, 10 a.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2734 - Switch docs from twiki to markdown
> 
> 
> Diffs
> -
> 
>   docs/pom.xml ae2e94c9c 
>   docs/src/site/markdown/AG_OozieLogging.md PRE-CREATION 
>   docs/src/site/markdown/ENG_Building.md PRE-CREATION 
>   docs/src/site/markdown/index.md PRE-CREATION 
>   docs/src/site/twiki/AG_ActionConfiguration.twiki 8c032a754 
>   docs/src/site/twiki/AG_HadoopConfiguration.twiki 528bf4a41 
>   docs/src/site/twiki/AG_Install.twiki b8031c82e 
>   docs/src/site/twiki/AG_Monitoring.twiki 425f55435 
>   docs/src/site/twiki/AG_OozieLogging.twiki ecdcfd33e 
>   docs/src/site/twiki/AG_OozieUpgrade.twiki 302406482 
>   docs/src/site/twiki/BundleFunctionalSpec.twiki 9749df597 
>   docs/src/site/twiki/CoordinatorFunctionalSpec.twiki cd416d4cd 
>   docs/src/site/twiki/DG_ActionAuthentication.twiki bbf2d57d3 
>   docs/src/site/twiki/DG_CommandLineTool.twiki 8d33c508e 
>   docs/src/site/twiki/DG_CoordinatorRerun.twiki fbb1376d5 
>   docs/src/site/twiki/DG_CustomActionExecutor.twiki 783148495 
>   docs/src/site/twiki/DG_DistCpActionExtension.twiki 8bab3da44 
>   docs/src/site/twiki/DG_EmailActionExtension.twiki 4de290ccf 
>   docs/src/site/twiki/DG_Examples.twiki 5323a1773 
>   docs/src/site/twiki/DG_FluentJobAPI.twiki c8b764bb0 
>   docs/src/site/twiki/DG_HCatalogIntegration.twiki d3107b453 
>   docs/src/site/twiki/DG_Hive2ActionExtension.twiki efbe56dab 
>   docs/src/site/twiki/DG_HiveActionExtension.twiki aaa74fa7b 
>   docs/src/site/twiki/DG_JMSNotifications.twiki a4b0f0d1a 
>   docs/src/site/twiki/DG_Overview.twiki 3ec94a2f1 
>   docs/src/site/twiki/DG_QuickStart.twiki d6a0069ac 
>   docs/src/site/twiki/DG_SLAMonitoring.twiki c91c227dc 
>   docs/src/site/twiki/DG_ShellActionExtension.twiki 5894c280a 
>   docs/src/site/twiki/DG_SparkActionExtension.twiki ce80e45fc 
>   docs/src/site/twiki/DG_SqoopActionExtension.twiki 0317d0cec 
>   docs/src/site/twiki/DG_SshActionExtension.twiki 5a51d49c5 
>   docs/src/site/twiki/DG_WorkflowReRun.twiki 88d982b75 
>   docs/src/site/twiki/ENG_Building.twiki b86102683 
>   

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

(Updated Sept. 7, 2018, 8:08 a.m.)


Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
Peter Bacsko.


Changes
---

Addressing review comments.


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
05fac3953eed5a5e2a16fa362656596f3a962a88 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMain.java 
c9e2a914a4b88640ac1c370b8971355dc087a235 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/10/

Changes: https://reviews.apache.org/r/68237/diff/9-10/


Testing
---

Tested on a real cluster, plus added test cases to 
`TestMapReduceActionExecutor` and new test classes.


Thanks,

András Piros

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

> On Sept. 6, 2018, 9:42 p.m., Peter Bacsko wrote:
> > core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java
> > Lines 377 (patched)
> > 
> >
> > What's the reason for the list? Couldn't it be just an ApplicationId 
> > reference?

Moved from 
[`TestSubWorkflowActionExecutor`](https://github.com/apache/oozie/blob/master/core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java#L547-L568)
 for code reuse.


> On Sept. 6, 2018, 9:42 p.m., Peter Bacsko wrote:
> > core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java
> > Lines 388 (patched)
> > 
> >
> > Does that file contains only a single line? If so, this piece of code 
> > can be simplified.

Moved from 
[`TestSubWorkflowActionExecutor`](https://github.com/apache/oozie/blob/master/core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java#L547-L568)
 for code reuse.


> On Sept. 6, 2018, 9:42 p.m., Peter Bacsko wrote:
> > core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
> > Lines 43 (patched)
> > 
> >
> > Interesting... don't you need to add 
> > @RunWith(MockitoJunitRunner.class)? Does JUnit4 automatically handle @Mock 
> > members?

Indeed, instead of explicit `Mockito#mock()` calls now using 
`@RunWith(MockitoJUnitRunner.class)`.


- András


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


On Sept. 6, 2018, 3:59 p.m., András Piros wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68237/
> ---
> 
> (Updated Sept. 6, 2018, 3:59 p.m.)
> 
> 
> Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
> Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
> treated as SUCCEEDED
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
> 05fac3953eed5a5e2a16fa362656596f3a962a88 
>   
> core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java
>  83a23f5220aa72ba15edc8b98ef80a74213fcee8 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
> f39bba2c691435354dac6da7794e5142b511d937 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
> a31079a41d30677d35a253a4a69505c21aa585f6 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
>  893405e38ad71d22460768b76ed03ac4d9a0b95d 
>   
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMain.java 
> c9e2a914a4b88640ac1c370b8971355dc087a235 
>   
> sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
>  f460b6bd11f60dfb397c6bba82be1427c2d1b570 
> 
> 
> Diff: https://reviews.apache.org/r/68237/diff/9/
> 
> 
> Testing
> ---
> 
> Tested on a real cluster, plus added test cases to 
> `TestMapReduceActionExecutor` and new test classes.
> 
> 
> Thanks,
> 
> András Piros
> 
>

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java
Lines 576-581 (patched)


On the one hand we have an unordered `List` but in the 
reality we need only a `String` of the biggest `ApplicationId`.

Extracted method and written Javadoc to make the purpose clearer. Used 
`Iterables#transform()` and `Ordering#max()` for clarity.



core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java
Lines 647-657 (patched)


Yes. Added Javadoc to make it clearer.


- András Piros


On Sept. 6, 2018, 3:59 p.m., András Piros wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68237/
> ---
> 
> (Updated Sept. 6, 2018, 3:59 p.m.)
> 
> 
> Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
> Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
> treated as SUCCEEDED
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
> 05fac3953eed5a5e2a16fa362656596f3a962a88 
>   
> core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java
>  83a23f5220aa72ba15edc8b98ef80a74213fcee8 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
> f39bba2c691435354dac6da7794e5142b511d937 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
> a31079a41d30677d35a253a4a69505c21aa585f6 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
>  893405e38ad71d22460768b76ed03ac4d9a0b95d 
>   
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMain.java 
> c9e2a914a4b88640ac1c370b8971355dc087a235 
>   
> sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
>  f460b6bd11f60dfb397c6bba82be1427c2d1b570 
> 
> 
> Diff: https://reviews.apache.org/r/68237/diff/9/
> 
> 
> Testing
> ---
> 
> Tested on a real cluster, plus added test cases to 
> `TestMapReduceActionExecutor` and new test classes.
> 
> 
> Thanks,
> 
> András Piros
> 
>

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

(Updated Sept. 6, 2018, 3:59 p.m.)


Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
Peter Bacsko.


Changes
---

Changes:

* the class `YarnApplicationIdComparator` now considers also `ApplicationId` 
instances with different cluster timestamps as different
* refactored 
`MapReduceActionExecutor$YarnApplicationIdFinder#fallbackToYarnChildOrExternalId()`
 not to call `YarnClient#getApplications()` directly because of performance 
issues on a busy cluster. Instead, call 
`LauncherMain#getChildYarnApplications()` that calls 
`ClientRMProxy#getApplications(GetApplicationRequest request)` instead, for 
better filtering options. This way, performance doesn't suffer even on busy 
YARN clusters


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
05fac3953eed5a5e2a16fa362656596f3a962a88 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMain.java 
c9e2a914a4b88640ac1c370b8971355dc087a235 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/9/

Changes: https://reviews.apache.org/r/68237/diff/8-9/


Testing
---

Tested on a real cluster, plus added test cases to 
`TestMapReduceActionExecutor` and new test classes.


Thanks,

András Piros

Re: Review Request 68628: OOZIE-2734 - Switch docs from twiki to markdown

[András Piros via Review Board]

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



Checked generated artifacts:

* `AG_ActionConfiguration.html`
* `AG_HadoopConfiguration.html`
* `AG_Install.html`


docs/src/site/twiki/AG_ActionConfiguration.twiki
Line 15 (original), 15 (patched)


Generated HTML looks like:
```
 and its value must follow the pattern _[AUTHORITY=ACTION-CONF-DIR_,]*. 
Where 
```



docs/src/site/twiki/AG_ActionConfiguration.twiki
Line 21 (original), 21 (patched)


Generated HTML:
```
All supported files in actionname/, e.g. 
actionname/.xml and actionname/*.properties (based on 
filename extension, sorted by lexical name, files with names lexically lower 
have lesser precedence than the following ones), if present.
```



docs/src/site/twiki/AG_ActionConfiguration.twiki
Line 29 (original), 29 (patched)


HTML



docs/src/site/twiki/AG_HadoopConfiguration.twiki
Lines 40-44 (original), 41-45 (patched)


html



docs/src/site/twiki/AG_HadoopConfiguration.twiki
Line 62 (original), 64 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Lines 243-245 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Lines 316-320 (original), 327-331 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Lines 323-362 (original), 334-373 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Lines 373-374 (original), 384-385 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Lines 377-378 (original), 388-389 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 580 (original), 597 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Lines 703-705 (original), 728-730 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 732 (original), 758 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 737 (original), 763 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 751 (original), 778 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Lines 763-767 (original), 791-795 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 773 (original), 801 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 782 (original), 811 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 807 (original), 837 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 860 (original), 894 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 904 (original), 941 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 922 (original), 960 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 1058 (original), 1100 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 1065 (original), 1107 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 1070 (original), 1112 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 1097 (original), 1140 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 1107 (original), 1151 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Lines 1163 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 1127 (original), 1173 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 1129 (original), 1175 (patched)


html



docs/src/site/twiki/AG_Install.twiki
Line 1131 

Re: Review Request 67885: POC: OOZIE-3160 PriorityDelayQueue put()/take() can cause significant CPU load due to busy waiting

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Sept. 4, 2018, 3:45 p.m., Peter Bacsko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67885/
> ---
> 
> (Updated Sept. 4, 2018, 3:45 p.m.)
> 
> 
> Review request for oozie, András Piros, Peter Cseh, Kinga Marton, and Robert 
> Kanter.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> See upstream JIRA for details
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java 
> PRE-CREATION 
>   core/src/main/java/org/apache/oozie/service/CallableQueueService.java 
> ef8d58da5 
>   core/src/main/java/org/apache/oozie/util/PriorityDelayQueue.java 75c20698c 
>   core/src/main/resources/oozie-default.xml b69d2c9aa 
>   core/src/test/java/org/apache/oozie/service/TestAsyncXCommandExecutor.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java 
> 9c2a11d6f 
> 
> 
> Diff: https://reviews.apache.org/r/67885/diff/10/
> 
> 
> Testing
> ---
> 
> 1. Executed TestCallableQueueService which passed completely.
> 2. New unit tests for ASyncXCommandExecutor.
> 3. Tried it on a 3-node cluster
> 
> 
> Thanks,
> 
> Peter Bacsko
> 
>

Re: Review Request 67885: POC: OOZIE-3160 PriorityDelayQueue put()/take() can cause significant CPU load due to busy waiting

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java
Lines 1 (patched)


Apache 2.0 license header missing.



core/src/main/java/org/apache/oozie/service/CallableQueueService.java
Lines 562 (patched)


Typo: `by looking at the`



core/src/test/java/org/apache/oozie/service/TestAsyncXCommandExecutor.java
Lines 75 (patched)


Remove newline


- András Piros


On Sept. 4, 2018, 12:05 p.m., Peter Bacsko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67885/
> ---
> 
> (Updated Sept. 4, 2018, 12:05 p.m.)
> 
> 
> Review request for oozie, András Piros, Peter Cseh, Kinga Marton, and Robert 
> Kanter.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> See upstream JIRA for details
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java 
> PRE-CREATION 
>   core/src/main/java/org/apache/oozie/service/CallableQueueService.java 
> ef8d58da5 
>   core/src/main/java/org/apache/oozie/util/PriorityDelayQueue.java 75c20698c 
>   core/src/main/resources/oozie-default.xml b69d2c9aa 
>   core/src/test/java/org/apache/oozie/service/TestAsyncXCommandExecutor.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java 
> 9c2a11d6f 
> 
> 
> Diff: https://reviews.apache.org/r/67885/diff/9/
> 
> 
> Testing
> ---
> 
> 1. Executed TestCallableQueueService which passed completely.
> 2. New unit tests for ASyncXCommandExecutor.
> 3. Tried it on a 3-node cluster
> 
> 
> Thanks,
> 
> Peter Bacsko
> 
>

Re: Review Request 68505: OOZIE-2877 Git action

[András Piros via Review Board]

> On Aug. 31, 2018, 10:03 a.m., Andras Salamon wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Line 134 (original), 134 (patched)
> > 
> >
> > Shouldn't we register this error code in registerErrors method?

ActionExecutor#registerError() is used when a specific Exception subclass 
should always be treated given the same errorType and errorCode. This is not 
the case here.


- András


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


On Aug. 31, 2018, 9:50 a.m., András Piros wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68505/
> ---
> 
> (Updated Aug. 31, 2018, 9:50 a.m.)
> 
> 
> Review request for oozie, Andras Salamon, Clay B., Peter Cseh, Kinga Marton, 
> and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2877 Git action
> 
> 
> Diffs
> -
> 
>   client/src/main/resources/git-action-1.0.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml 
> b69d2c9aacc1a7f5f903fb80f77bbf5482feb3b5 
>   core/src/test/java/org/apache/oozie/test/XTestCase.java 
> 661970d57ac038d7311856a93f5221f081e9ce15 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 
> 76cbe21ecd79eae86885a8db77831bf2c40f2d4f 
>   examples/src/main/apps/git/job.properties PRE-CREATION 
>   examples/src/main/apps/git/workflow.xml PRE-CREATION 
>   examples/src/main/java/org/apache/oozie/example/fluentjob/Git.java 
> PRE-CREATION 
>   fluent-job/fluent-job-api/pom.xml cefd43c143550a24ffba5a1d6922b8241dd5756a 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/DistcpConfigurationConverter.java
>  fffb734e7c1b920e94f76e6a2b3062dd5aa73a86 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5cbbffbee02785361eb2bbde2fc1c31e7a 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/Hive2ConfigurationConverter.java
>  c67b5aeab41e0c2d963c0ad3cf44fd19fe7c66e3 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/HiveConfigurationConverter.java
>  5f9a2b180623b89c793e8e6bb6e03d445c0ad991 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/InlineWorkflowConfigurationConverter.java
>  b1e17c9b5b8c29a81792ebfc637a53564bdb60fe 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ShellConfigurationConverter.java
>  ab73dfd2f24cbbfa60a37d75e5057f3ff2a1c0fa 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/SparkConfigurationConverter.java
>  8827769ea84f8f08a79e5e2d0189e2d1ded4bd9d 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/SqoopConfigurationConverter.java
>  1d4f615af583f8b941e570f3e3f239979cc8f825 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554ba4cd313a9d5ab8472ab3c62b6bc17380 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml 
> a5f890eef9ccc352a5a852781f4212d96913e6d2 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 
> 48f68905ac2df2818d88907b26e863326d6d2c61 
>   
> fluent-job/fluent-job-client/src/test/java/org/apache/oozie/jobs/client/minitest/TestGitAction.java
>  PRE-CREATION 
>   pom.xml e0dbe85e5238fdc11cbd44cc31f65fc768c7454d 
>   sharelib/git/pom.xml PRE-CREATION 
>   sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java 
> PRE-CREATION 
>   
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java 
> PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java 
> PRE-CREATION 
>   
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
>  PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java 
> PRE-CREATION 
>   
> 

Re: Review Request 68140: OOZIE-3229 Improved filtering options in V2SLAServlet

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/FilterParser.java
Lines 49 (patched)


Use `PARAMETER_EQUALS`



core/src/main/java/org/apache/oozie/FilterParser.java
Lines 56 (patched)


`Strings.isNullOrEmpty()`



core/src/main/java/org/apache/oozie/FilterParser.java
Lines 74-78 (patched)


Here we could give a bit more context to the caller, we already have that



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Line 57 (original), 85 (patched)


Javadoc comment on this `Pattern`, plus I think the Ascii part 
(`oozie-${USER_NAME_TRUNCATED_TO_$_CHARS}`) cannot be of arbitrary length.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 118-120 (patched)


Could be `final`



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 122 (patched)


Could be `final`



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 128 (patched)


Can be `final`



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 130 (patched)


`field.type.equals()`



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 133 (patched)


`field.type.equals()`



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 137 (patched)


`field.type.equals()`



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 213-214 (patched)


Would extract to some method like `ensureCriteriaFields()`



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 215-216 (patched)


Extract to `createSelectFrom()`



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 218 (patched)


Always `ORDER BY NOMINAL_TIME`?



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 228-239 (patched)


I'm thinking whether `fieldName LIKE '%fieldValue%'` can / should be 
supported.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 255 (patched)


`FilterField.PARENT_ID.dbFieldName`



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 273 (patched)


Instead, code would be a bit cleaner if following would be used:
```
if (bundle == null) {
  return;
}
```



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 295 (patched)


Special handling of unparseable `event_status` value



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 296-354 (patched)


Isn't it possible to use the Visitor design pattern instead? Or, at the 
very least, extract the `case` contents to well named methods?



core/src/test/java/org/apache/oozie/TestFilterParser.java
Lines 30 (patched)


Needn't and shouldn't `extends XTestCase`



core/src/test/java/org/apache/oozie/TestFilterParser.java
Lines 45 (patched)


Rather use `ExpectedException`



core/src/test/java/org/apache/oozie/TestFilterParser.java
Lines 47-50 (patched)


Instead of this comment, rather use a name like `ignored`



core/src/test/java/org/apache/oozie/TestFilterParser.java
Lines 57 (patched)


Rather use `ExpectedException`



core/src/test/java/org/apache/oozie/TestFilterParser.java
Lines 59-62 (patched)

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

(Updated Sept. 3, 2018, 9:10 a.m.)


Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
8f0f244013932476d8ae454d224f235948529b34 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/8/


Testing (updated)
---

Tested on a real cluster, plus added test cases to 
`TestMapReduceActionExecutor` and new test classes.


Thanks,

András Piros

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

(Updated Sept. 3, 2018, 7:59 a.m.)


Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
Peter Bacsko.


Changes
---

More comments on `MapReduceActionExecutor` and `TestMapReduceActionExecutor` 
classes.


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
8f0f244013932476d8ae454d224f235948529b34 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/8/

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


Testing
---

Tested on a real cluster, plus added test cases to 
`TestMapReduceActionExecutor` and new test classes.

Please note that I could find no proper way of actually getting MapReduce to 
start a new job while using `OozieClient#submit()` - apparently no YARN child 
application is created. Please advise what's the best way to advance, maybe 
call `JobClient#submitJob()` directly from 
`ActionExecutorTestCase#startWorkflowAndFailChildMRJob()`.


Thanks,

András Piros

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

(Updated Aug. 31, 2018, 10:50 a.m.)


Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
Peter Bacsko.


Changes
---

Addressing review comments.


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
8f0f244013932476d8ae454d224f235948529b34 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/7/

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


Testing
---

Tested on a real cluster, plus added test cases to 
`TestMapReduceActionExecutor` and new test classes.

Please note that I could find no proper way of actually getting MapReduce to 
start a new job while using `OozieClient#submit()` - apparently no YARN child 
application is created. Please advise what's the best way to advance, maybe 
call `JobClient#submitJob()` directly from 
`ActionExecutorTestCase#startWorkflowAndFailChildMRJob()`.


Thanks,

András Piros

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

> On Aug. 30, 2018, 1:34 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java
> > Lines 575 (patched)
> > 
> >
> > Can't we just compare the IDs as strings? What's the rationale of this 
> > class?
> > 
> > If it's not passed to other data structures or classes which use a 
> > Comparator, then I guess it doesn't have to implement it.

The rationale behind this class is to compare two YARN application ID 
`String`s, when the cluster startup time is different. Newer application IDs 
will be treated as "greater".

We cannot really use plain `String` comparison because we could face situations 
where different application IDs are read with different cluster startup times.


- András


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


On Aug. 24, 2018, 2:18 p.m., András Piros wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68237/
> ---
> 
> (Updated Aug. 24, 2018, 2:18 p.m.)
> 
> 
> Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
> Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
> treated as SUCCEEDED
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
> 8f0f244013932476d8ae454d224f235948529b34 
>   
> core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java
>  83a23f5220aa72ba15edc8b98ef80a74213fcee8 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
> f39bba2c691435354dac6da7794e5142b511d937 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
> a31079a41d30677d35a253a4a69505c21aa585f6 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
>  893405e38ad71d22460768b76ed03ac4d9a0b95d 
>   
> sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
>  f460b6bd11f60dfb397c6bba82be1427c2d1b570 
> 
> 
> Diff: https://reviews.apache.org/r/68237/diff/6/
> 
> 
> Testing
> ---
> 
> Tested on a real cluster, plus added test cases to 
> `TestMapReduceActionExecutor` and new test classes.
> 
> Please note that I could find no proper way of actually getting MapReduce to 
> start a new job while using `OozieClient#submit()` - apparently no YARN child 
> application is created. Please advise what's the best way to advance, maybe 
> call `JobClient#submitJob()` directly from 
> `ActionExecutorTestCase#startWorkflowAndFailChildMRJob()`.
> 
> 
> Thanks,
> 
> András Piros
> 
>

Re: Review Request 68505: OOZIE-2877 Git action

[András Piros via Review Board]

> On Aug. 31, 2018, 10:03 a.m., Andras Salamon wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Line 175 (original), 175 (patched)
> > 
> >
> > Shouldn't we register this error code in registerErrors method?

`ActionExecutor#registerError()` is used when a specific `Exception` subclass 
should always be treated given the same `errorType` and `errorCode`. This is 
not the case here.


> On Aug. 31, 2018, 10:03 a.m., Andras Salamon wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Line 183 (original), 183 (patched)
> > 
> >
> > Shouldn't we register this error code in registerErrors method?

`ActionExecutor#registerError()` is used when a specific `Exception` subclass 
should always be treated given the same `errorType` and `errorCode`. This is 
not the case here.


> On Aug. 31, 2018, 10:03 a.m., Andras Salamon wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java
> > Line 106 (original), 107 (patched)
> > 
> >
> > If we want to check the length then assertEquals("...", 
> > credentialFileData.length(), testOutput.length()) would be better

This is how it looked like before Peter Bacsko pointed out it's not a good idea 
to check for a given length of a test output. So we're checking here only that 
the file is indeed created and non-empty.


- András


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


On Aug. 31, 2018, 9:50 a.m., András Piros wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68505/
> ---
> 
> (Updated Aug. 31, 2018, 9:50 a.m.)
> 
> 
> Review request for oozie, Andras Salamon, Clay B., Peter Cseh, Kinga Marton, 
> and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2877 Git action
> 
> 
> Diffs
> -
> 
>   client/src/main/resources/git-action-1.0.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml 
> b69d2c9aacc1a7f5f903fb80f77bbf5482feb3b5 
>   core/src/test/java/org/apache/oozie/test/XTestCase.java 
> 661970d57ac038d7311856a93f5221f081e9ce15 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 
> 76cbe21ecd79eae86885a8db77831bf2c40f2d4f 
>   examples/src/main/apps/git/job.properties PRE-CREATION 
>   examples/src/main/apps/git/workflow.xml PRE-CREATION 
>   examples/src/main/java/org/apache/oozie/example/fluentjob/Git.java 
> PRE-CREATION 
>   fluent-job/fluent-job-api/pom.xml cefd43c143550a24ffba5a1d6922b8241dd5756a 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/DistcpConfigurationConverter.java
>  fffb734e7c1b920e94f76e6a2b3062dd5aa73a86 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5cbbffbee02785361eb2bbde2fc1c31e7a 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/Hive2ConfigurationConverter.java
>  c67b5aeab41e0c2d963c0ad3cf44fd19fe7c66e3 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/HiveConfigurationConverter.java
>  5f9a2b180623b89c793e8e6bb6e03d445c0ad991 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/InlineWorkflowConfigurationConverter.java
>  b1e17c9b5b8c29a81792ebfc637a53564bdb60fe 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ShellConfigurationConverter.java
>  ab73dfd2f24cbbfa60a37d75e5057f3ff2a1c0fa 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/SparkConfigurationConverter.java
>  8827769ea84f8f08a79e5e2d0189e2d1ded4bd9d 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/SqoopConfigurationConverter.java
>  1d4f615af583f8b941e570f3e3f239979cc8f825 
>   
> 

Re: Review Request 68505: OOZIE-2877 Git action

[András Piros via Review Board]

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

(Updated Aug. 31, 2018, 9:50 a.m.)


Review request for oozie, Andras Salamon, Clay B., Peter Cseh, Kinga Marton, 
and Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-2877 Git action


Diffs (updated)
-

  client/src/main/resources/git-action-1.0.xsd PRE-CREATION 
  core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
PRE-CREATION 
  core/src/main/resources/oozie-default.xml 
b69d2c9aacc1a7f5f903fb80f77bbf5482feb3b5 
  core/src/test/java/org/apache/oozie/test/XTestCase.java 
661970d57ac038d7311856a93f5221f081e9ce15 
  docs/src/site/twiki/WorkflowFunctionalSpec.twiki 
76cbe21ecd79eae86885a8db77831bf2c40f2d4f 
  examples/src/main/apps/git/job.properties PRE-CREATION 
  examples/src/main/apps/git/workflow.xml PRE-CREATION 
  examples/src/main/java/org/apache/oozie/example/fluentjob/Git.java 
PRE-CREATION 
  fluent-job/fluent-job-api/pom.xml cefd43c143550a24ffba5a1d6922b8241dd5756a 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/DistcpConfigurationConverter.java
 fffb734e7c1b920e94f76e6a2b3062dd5aa73a86 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
 7bb82e5cbbffbee02785361eb2bbde2fc1c31e7a 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/Hive2ConfigurationConverter.java
 c67b5aeab41e0c2d963c0ad3cf44fd19fe7c66e3 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/HiveConfigurationConverter.java
 5f9a2b180623b89c793e8e6bb6e03d445c0ad991 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/InlineWorkflowConfigurationConverter.java
 b1e17c9b5b8c29a81792ebfc637a53564bdb60fe 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ShellConfigurationConverter.java
 ab73dfd2f24cbbfa60a37d75e5057f3ff2a1c0fa 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/SparkConfigurationConverter.java
 8827769ea84f8f08a79e5e2d0189e2d1ded4bd9d 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/SqoopConfigurationConverter.java
 1d4f615af583f8b941e570f3e3f239979cc8f825 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
 ec56554ba4cd313a9d5ab8472ab3c62b6bc17380 
  fluent-job/fluent-job-api/src/main/resources/action_mappings.xml 
a5f890eef9ccc352a5a852781f4212d96913e6d2 
  fluent-job/fluent-job-api/src/main/xjb/bindings.xml 
48f68905ac2df2818d88907b26e863326d6d2c61 
  
fluent-job/fluent-job-client/src/test/java/org/apache/oozie/jobs/client/minitest/TestGitAction.java
 PRE-CREATION 
  pom.xml e0dbe85e5238fdc11cbd44cc31f65fc768c7454d 
  sharelib/git/pom.xml PRE-CREATION 
  sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java 
PRE-CREATION 
  sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java 
PRE-CREATION 
  sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java 
PRE-CREATION 
  
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
 PRE-CREATION 
  sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java 
PRE-CREATION 
  
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
 PRE-CREATION 
  sharelib/pom.xml 39cea257e750eb8b3f3c4cb3f137093fccc03016 
  src/main/assemblies/sharelib.xml 07dc69c8276895b254e5af0cc021cce6ebad18f4 
  webapp/pom.xml fd3f89feedd76657f1431898a268298549826d6f 


Diff: https://reviews.apache.org/r/68505/diff/3/

Changes: https://reviews.apache.org/r/68505/diff/2-3/


Testing
---

Multiple unit and integration tests, as well as tests performed on a local 
Oozie server w/ pseudo-distributed Hadoop.

Based on the work Clay B. has done in previous patchsets till 
[OOZIE-2877.014-3.patch](https://issues.apache.org/jira/secure/attachment/12934336/OOZIE-2877.014-3.patch).


Thanks,

András Piros

Re: Review Request 68505: OOZIE-2877 Git action

[András Piros via Review Board]

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

(Updated Aug. 30, 2018, 6:42 p.m.)


Review request for oozie, Andras Salamon, Clay B., Peter Cseh, Kinga Marton, 
and Peter Bacsko.


Changes
---

Addressing review comments.


Repository: oozie-git


Description
---

OOZIE-2877 Git action


Diffs (updated)
-

  client/src/main/resources/git-action-1.0.xsd PRE-CREATION 
  core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
PRE-CREATION 
  core/src/main/resources/oozie-default.xml 
b69d2c9aacc1a7f5f903fb80f77bbf5482feb3b5 
  core/src/test/java/org/apache/oozie/test/XTestCase.java 
661970d57ac038d7311856a93f5221f081e9ce15 
  docs/src/site/twiki/WorkflowFunctionalSpec.twiki 
76cbe21ecd79eae86885a8db77831bf2c40f2d4f 
  examples/src/main/apps/git/job.properties PRE-CREATION 
  examples/src/main/apps/git/workflow.xml PRE-CREATION 
  examples/src/main/java/org/apache/oozie/example/fluentjob/Git.java 
PRE-CREATION 
  fluent-job/fluent-job-api/pom.xml cefd43c143550a24ffba5a1d6922b8241dd5756a 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/DistcpConfigurationConverter.java
 fffb734e7c1b920e94f76e6a2b3062dd5aa73a86 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
 7bb82e5cbbffbee02785361eb2bbde2fc1c31e7a 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/Hive2ConfigurationConverter.java
 c67b5aeab41e0c2d963c0ad3cf44fd19fe7c66e3 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/HiveConfigurationConverter.java
 5f9a2b180623b89c793e8e6bb6e03d445c0ad991 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/InlineWorkflowConfigurationConverter.java
 b1e17c9b5b8c29a81792ebfc637a53564bdb60fe 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ShellConfigurationConverter.java
 ab73dfd2f24cbbfa60a37d75e5057f3ff2a1c0fa 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/SparkConfigurationConverter.java
 8827769ea84f8f08a79e5e2d0189e2d1ded4bd9d 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/SqoopConfigurationConverter.java
 1d4f615af583f8b941e570f3e3f239979cc8f825 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
 ec56554ba4cd313a9d5ab8472ab3c62b6bc17380 
  fluent-job/fluent-job-api/src/main/resources/action_mappings.xml 
a5f890eef9ccc352a5a852781f4212d96913e6d2 
  fluent-job/fluent-job-api/src/main/xjb/bindings.xml 
48f68905ac2df2818d88907b26e863326d6d2c61 
  
fluent-job/fluent-job-client/src/test/java/org/apache/oozie/jobs/client/minitest/TestGitAction.java
 PRE-CREATION 
  pom.xml e0dbe85e5238fdc11cbd44cc31f65fc768c7454d 
  sharelib/git/pom.xml PRE-CREATION 
  sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java 
PRE-CREATION 
  sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java 
PRE-CREATION 
  sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java 
PRE-CREATION 
  
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
 PRE-CREATION 
  sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java 
PRE-CREATION 
  
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
 PRE-CREATION 
  sharelib/pom.xml 39cea257e750eb8b3f3c4cb3f137093fccc03016 
  src/main/assemblies/sharelib.xml 07dc69c8276895b254e5af0cc021cce6ebad18f4 
  webapp/pom.xml fd3f89feedd76657f1431898a268298549826d6f 


Diff: https://reviews.apache.org/r/68505/diff/2/

Changes: https://reviews.apache.org/r/68505/diff/1-2/


Testing
---

Multiple unit and integration tests, as well as tests performed on a local 
Oozie server w/ pseudo-distributed Hadoop.

Based on the work Clay B. has done in previous patchsets till 
[OOZIE-2877.014-3.patch](https://issues.apache.org/jira/secure/attachment/12934336/OOZIE-2877.014-3.patch).


Thanks,

András Piros

Re: Review Request 68517: OOZIE 3210 - revision information is empty

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Aug. 27, 2018, 2:45 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68517/
> ---
> 
> (Updated Aug. 27, 2018, 2:45 p.m.)
> 
> 
> Review request for oozie, András Piros, Attila Sasvari, and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE 3210 - revision information is empty
> 
> 
> Diffs
> -
> 
>   bin/create-release-artifact 86d4d5df 
> 
> 
> Diff: https://reviews.apache.org/r/68517/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Review Request 68505: OOZIE-2877 Git action

[András Piros via Review Board]

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

Review request for oozie, Andras Salamon, Clay B., Peter Cseh, Kinga Marton, 
and Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-2877 Git action


Diffs
-

  client/src/main/resources/git-action-1.0.xsd PRE-CREATION 
  core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
PRE-CREATION 
  core/src/main/resources/oozie-default.xml 
b69d2c9aacc1a7f5f903fb80f77bbf5482feb3b5 
  core/src/test/java/org/apache/oozie/test/XTestCase.java 
661970d57ac038d7311856a93f5221f081e9ce15 
  docs/src/site/twiki/WorkflowFunctionalSpec.twiki 
76cbe21ecd79eae86885a8db77831bf2c40f2d4f 
  examples/src/main/apps/git/job.properties PRE-CREATION 
  examples/src/main/apps/git/workflow.xml PRE-CREATION 
  examples/src/main/java/org/apache/oozie/example/fluentjob/Git.java 
PRE-CREATION 
  fluent-job/fluent-job-api/pom.xml cefd43c143550a24ffba5a1d6922b8241dd5756a 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/DistcpConfigurationConverter.java
 fffb734e7c1b920e94f76e6a2b3062dd5aa73a86 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
 7bb82e5cbbffbee02785361eb2bbde2fc1c31e7a 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
 PRE-CREATION 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/Hive2ConfigurationConverter.java
 c67b5aeab41e0c2d963c0ad3cf44fd19fe7c66e3 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/HiveConfigurationConverter.java
 5f9a2b180623b89c793e8e6bb6e03d445c0ad991 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/InlineWorkflowConfigurationConverter.java
 b1e17c9b5b8c29a81792ebfc637a53564bdb60fe 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ShellConfigurationConverter.java
 ab73dfd2f24cbbfa60a37d75e5057f3ff2a1c0fa 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/SparkConfigurationConverter.java
 8827769ea84f8f08a79e5e2d0189e2d1ded4bd9d 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/SqoopConfigurationConverter.java
 1d4f615af583f8b941e570f3e3f239979cc8f825 
  
fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
 ec56554ba4cd313a9d5ab8472ab3c62b6bc17380 
  fluent-job/fluent-job-api/src/main/resources/action_mappings.xml 
a5f890eef9ccc352a5a852781f4212d96913e6d2 
  fluent-job/fluent-job-api/src/main/xjb/bindings.xml 
48f68905ac2df2818d88907b26e863326d6d2c61 
  
fluent-job/fluent-job-client/src/test/java/org/apache/oozie/jobs/client/minitest/TestGitAction.java
 PRE-CREATION 
  pom.xml e0dbe85e5238fdc11cbd44cc31f65fc768c7454d 
  sharelib/git/pom.xml PRE-CREATION 
  sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java 
PRE-CREATION 
  sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java 
PRE-CREATION 
  sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java 
PRE-CREATION 
  
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
 PRE-CREATION 
  sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java 
PRE-CREATION 
  
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
 PRE-CREATION 
  sharelib/pom.xml 39cea257e750eb8b3f3c4cb3f137093fccc03016 
  src/main/assemblies/sharelib.xml 07dc69c8276895b254e5af0cc021cce6ebad18f4 
  webapp/pom.xml fd3f89feedd76657f1431898a268298549826d6f 


Diff: https://reviews.apache.org/r/68505/diff/1/


Testing
---

Multiple unit and integration tests, as well as tests performed on a local 
Oozie server w/ pseudo-distributed Hadoop.

Based on the work Clay B. has done in previous patchsets till 
[OOZIE-2877.014-3.patch](https://issues.apache.org/jira/secure/attachment/12934336/OOZIE-2877.014-3.patch).


Thanks,

András Piros

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On May 30, 2017, 11:08 a.m., András Piros wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
> > Lines 60 (patched)
> > 
> >
> > Are system props reset after this test run?
> 
> Clay B. wrote:
> I am not sure what the entire chain of `ActionExecutorTestCase`'s methods 
> are doing. However, I am not explicitly clearing this to my knowledge; do you 
> expect issues? (For reference, it seems 
> [`TestJavaActionExecutor()`](https://github.com/apache/oozie/blob/5998c18fde1da769e91e3ef1bcca484723730c76/core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java#L90)
>  is not clearing these either?)
> 
> András Piros wrote:
> I'd rather not set any system properties here but go for 
> [`Services.get().getConf().set()`](https://github.com/apache/oozie/blob/eb168360c550943828d9fe2c7bfdcd4f5e830003/core/src/test/java/org/apache/oozie/service/TestActionService.java#L52-L53),
>  if that works.
> 
> If not, I'd still set this system property in `@Before void setUp()` and 
> reset in `@After void tearDown()` as this one is used by other important 
> pieces of code.
> 
> Clay B. wrote:
> It seems this idiom is followed by the following and I am missing where 
> they do clean-up?
> 
> * 
> [Pig](https://github.com/apache/oozie/blob/master/sharelib/pig/src/test/java/org/apache/oozie/action/hadoop/TestPigActionExecutor.java#L81-L85)
> * 
> [Spark](https://github.com/apache/oozie/blob/299370b44e2d7b22bfe622d19f65b02c5b18282f/sharelib/spark/src/test/java/org/apache/oozie/action/hadoop/TestSparkActionExecutor.java#L66-L69)
> * 
> [MapReduce](https://github.com/apache/oozie/blob/5e1c9d362afe1b2c6423a386aeac7f04d3337f65/sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java#L86-L90)

Thanks for the explanation, dropping this issue.


- András


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


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 p.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Bugs: OOZIE-2877
> https://issues.apache.org/jira/browse/OOZIE-2877
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2877 - Oozie Git Action
> 
> 
> Diffs
> -
> 
>   client/src/main/resources/git-action-1.0.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml b69d2c9 
>   core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21 
>   examples/src/main/apps/git/job.properties PRE-CREATION 
>   examples/src/main/apps/git/workflow.xml PRE-CREATION 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   sharelib/git/pom.xml PRE-CREATION 
>   sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java 
> PRE-CREATION 
>   
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java 
> PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java 
> PRE-CREATION 
>   
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
>  PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java 
> PRE-CREATION 
>   
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
>  PRE-CREATION 
>   sharelib/pom.xml 6a0864d 
>   src/main/assemblies/sharelib.xml 07dc69c 
>   webapp/pom.xml fd3f89f 
> 
> 
> Diff: 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On April 30, 2018, 3:12 p.m., Peter Cseh wrote:
> > client/src/main/resources/git-action-0.1.xsd
> > Lines 32 (patched)
> > 
> >
> > I don't know if it's possible to define uris outside 
> > oozie.service.HadoopAccessorService.nameNode.whitelist here.
> > 
> > Can you please add a test for that?
> 
> Clay B. wrote:
> Thanks Peter for thinking through this. I do not know that it be 
> necessary we allow URI's other than the HadoopAccessorService whitelisted 
> nameservices? Andras had a request that I rename `cloneRepoToHdfs` to be less 
> opinionated but I presumed it would still be a whitelisted HCFS of some sort. 
> For a test here, are you thinking of me using a `file://` path for 
> `testWhenRepoIsClonedThenGitIndexContentIsReadSuccessfully()` in 
> `TestIntegrationGitActionExecutor.java` or can you provide me a bit more 
> guidance or the use-case?

@Peter Cseh it's already covered by `HadoopAccessorService#createFileSystem()` 
that to latter end is called by `JavaActionExecutor#setupActionConf()` as well. 
So no need to cover that separately.


> On April 30, 2018, 3:12 p.m., Peter Cseh wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 137-145 (patched)
> > 
> >
> > I wonder how strict we should be with credential files. We might want 
> > to do a ssh-like check. Ssh fails if your .pem file is not readably only by 
> > your user, that's why we're setting the permissions here.
> > 
> > At least we could print out a warning to help users avoid leaking 
> > credentials to everyone from HDFS
> > 
> > Or we can go ssh-level strict and have an option to do fall back to 
> > warnings only.
> 
> Clay B. wrote:
> Good idea! Indeed, I've had a hard time getting users to "do the right 
> thing" in the past. As you see many more environments than I do, would you 
> recommend we also check HDFS extended ACLs as well if available? Would you 
> want an "allow insecure credential" boolean or would you want something more 
> for the override?

@Peter Cseh can you please give an answer? Thanks!


- András


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


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 p.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Bugs: OOZIE-2877
> https://issues.apache.org/jira/browse/OOZIE-2877
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2877 - Oozie Git Action
> 
> 
> Diffs
> -
> 
>   client/src/main/resources/git-action-1.0.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml b69d2c9 
>   core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21 
>   examples/src/main/apps/git/job.properties PRE-CREATION 
>   examples/src/main/apps/git/workflow.xml PRE-CREATION 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   sharelib/git/pom.xml PRE-CREATION 
>   sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java 
> PRE-CREATION 
>   
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java 
> PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java 
> PRE-CREATION 
>   
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
>  PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Lines 182-183 (patched)
> > 
> >
> > These nested ifs can be merged.

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 200-201 (patched)
> > 
> >
> > Do we need this?

Removed.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java
> > Lines 92 (patched)
> > 
> >
> > Typo: "Checkiging"

Fixed.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
> > Lines 114 (patched)
> > 
> >
> > Better error message would be "Expected ActionExecutorException"

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
> > Lines 117 (patched)
> > 
> >
> > Better error message would be "Unexpected exception message" or sth 
> > like that

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
> > Lines 148-154 (patched)
> > 
> >
> > What are we testing here exactly?
> > 
> > I can't see any assert() calls.

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
> > Lines 183-188 (patched)
> > 
> >
> > Same here

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java
> > Lines 101-104 (patched)
> > 
> >
> > This should be replaced with a waitFor() method call - see Oozie 
> > codebase for examples.

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
> > Lines 30 (patched)
> > 
> >
> > Unused import

Done.


> On Aug. 7, 2018, 12:38 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
> > Lines 86 (patched)
> > 
> >
> > readContent variable is unused

Done.


- András


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


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 p.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Bugs: OOZIE-2877
> https://issues.apache.org/jira/browse/OOZIE-2877
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2877 - Oozie Git Action
> 
> 
> Diffs
> -
> 
>   client/src/main/resources/git-action-1.0.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml b69d2c9 
>   core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21 
>   examples/src/main/apps/git/job.properties PRE-CREATION 
>   examples/src/main/apps/git/workflow.xml PRE-CREATION 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On June 5, 2018, 4:46 p.m., Robert Kanter wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 216 (patched)
> > 
> >
> > Is this necessary?  Why not simply throw whatever exception 
> > (IOException, IllegalArgumentException, etc)?  We don't seem to be doing 
> > any special handling or anything for GitMainExceptions.
> 
> Clay B. wrote:
> I was simply copying the idiom from JavaMain but do not have any broader 
> plan for the exception; the same is true for `GitOperationsException`.
> 
> Robert Kanter wrote:
> I forget exactly the reason off the top of my head, but we needed to wrap 
> Exceptions in another layer in the Java Action to handle a certain case.  I 
> believe it was related to the fact that we'd be running unknown Java code 
> (provided by the user), but I'm not sure.  If you look at LauncherAM, it's 
> actually doing something with JavaMainException.  Anyway, you can see that 
> none of the other actions do this, so I don't think we need a 
> GitMainException.

Removed `GitMainException`.


- András


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


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 p.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Bugs: OOZIE-2877
> https://issues.apache.org/jira/browse/OOZIE-2877
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2877 - Oozie Git Action
> 
> 
> Diffs
> -
> 
>   client/src/main/resources/git-action-1.0.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml b69d2c9 
>   core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21 
>   examples/src/main/apps/git/job.properties PRE-CREATION 
>   examples/src/main/apps/git/workflow.xml PRE-CREATION 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   sharelib/git/pom.xml PRE-CREATION 
>   sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java 
> PRE-CREATION 
>   
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java 
> PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java 
> PRE-CREATION 
>   
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
>  PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java 
> PRE-CREATION 
>   
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
>  PRE-CREATION 
>   sharelib/pom.xml 6a0864d 
>   src/main/assemblies/sharelib.xml 07dc69c 
>   webapp/pom.xml fd3f89f 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/10/
> 
> 
> Testing
> ---
> 
> Tested using unit and integration tests. Still need to:
> * Test on a cluster
> * Test with an authenticated SSH hosted Git repo
> 
> Sumitted a request to the JGit community as their branch pulling code seems 
> to have an 
> [issue](https://dev.eclipse.org/mhonarc/lists/jgit-dev/msg03343.html).
> 
> Andras has tested against his patch 011 with results at 
> https://issues.apache.org/jira/browse/OOZIE-2877?focusedCommentId=16459523=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16459523
> 
> 
> File Attachments
> 
> 
> 0001-OOZIE-2877-Oozie-Git-Action.patch
>   
> https://reviews.apache.org/media/uploaded/files/2017/05/29/24f90a78-3dc1-49fe-bf29-5927a3cd5e72__0001-OOZIE-2877-Oozie-Git-Action.patch
> Patch
>   
> 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On Aug. 8, 2018, 9:57 a.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Lines 152 (patched)
> > 
> >
> > Better naming suggested: ActionConfVerifier

Done.


> On Aug. 8, 2018, 9:57 a.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 38 (patched)
> > 
> >
> > General thoughts: this class has quite a few protected methods. We have 
> > to think about whether it's necessary or not. If the class is not going to 
> > be subclassed, making them "private" is preferable.

Done.


> On Aug. 8, 2018, 9:57 a.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 43 (patched)
> > 
> >
> > I'd prefer this as being private, with having a package private setter 
> > method.

Done.


> On Aug. 8, 2018, 9:57 a.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java
> > Lines 125 (patched)
> > 
> >
> > This string can be placed directly in the constructor.

Done.


> On Aug. 8, 2018, 9:57 a.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java
> > Lines 74 (patched)
> > 
> >
> > At least a warning/error message would be good here.

Done.


- András


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


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 p.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Bugs: OOZIE-2877
> https://issues.apache.org/jira/browse/OOZIE-2877
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2877 - Oozie Git Action
> 
> 
> Diffs
> -
> 
>   client/src/main/resources/git-action-1.0.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml b69d2c9 
>   core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21 
>   examples/src/main/apps/git/job.properties PRE-CREATION 
>   examples/src/main/apps/git/workflow.xml PRE-CREATION 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   sharelib/git/pom.xml PRE-CREATION 
>   sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java 
> PRE-CREATION 
>   
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java 
> PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java 
> PRE-CREATION 
>   
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
>  PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java 
> PRE-CREATION 
>   
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
>  PRE-CREATION 
>   sharelib/pom.xml 6a0864d 
>   src/main/assemblies/sharelib.xml 07dc69c 
>   webapp/pom.xml fd3f89f 
> 
> 
> Diff: https://reviews.apache.org/r/59620/diff/10/
> 
> 
> Testing
> ---
> 
> Tested using unit and integration tests. Still need to:
> * Test on a cluster
> * Test with an authenticated SSH hosted Git repo
> 
> Sumitted a request to the JGit community as their branch pulling code seems 
> to 

Re: Review Request 59620: This review board request is for an action to provide a Git action for Oozie

[András Piros via Review Board]

> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > docs/src/site/twiki/WorkflowFunctionalSpec.twiki
> > Lines 1667-1669 (patched)
> > 
> >
> > Do we really need to access a different cluster other than the one 
> > we're running on?
> > 
> > If key is located on a separate cluster which is secure we won't be 
> > allowed to access that file without proper credentials. Basically it's the 
> > same problem that appears under disctp. It's really not self-explanatory to 
> > setup cross-cluster authentication.
> > 
> > Therefore I'm against it. Also, the name node is available from 
> > "fs.defaultFS" property.
> 
> Clay B. wrote:
> I could certainly see someone storing a credential on a secure object 
> store (e.g. S3-like FS).
> 
> Peter Bacsko wrote:
> In that case, we have to mention in the docs that access control on the 
> remote side must be properly configured. Eg. if it's a hdfs:// URL that 
> points to an non-secure cluster, appropriate HDFS permissions are still 
> necessary.

A paragraph added to the docs.


> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 41 (patched)
> > 
> >
> > We don't use XLog inside LauncherMain, only Sysout. This will change in 
> > the future, but for now, let's stick to the conventions that we have 
> > already.
> 
> Clay B. wrote:
> I have left the help methods to leave logging level intention in place 
> but removed all references to XLog. Would you prefer the helpers to go too?
> 
> Peter Bacsko wrote:
> We haven't settled down on a logging solution. Geza suggested JDK loggers 
> (so that they don't conflict with log4j/log4j2/logback, whatever we migrate 
> to), but it's not finalized yet.

`XLog` references are not more present in `GitMain`.


- András


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


On Aug. 3, 2018, 10 p.m., Clay B. wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> ---
> 
> (Updated Aug. 3, 2018, 10 p.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Bugs: OOZIE-2877
> https://issues.apache.org/jira/browse/OOZIE-2877
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-2877 - Oozie Git Action
> 
> 
> Diffs
> -
> 
>   client/src/main/resources/git-action-1.0.xsd PRE-CREATION 
>   core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java 
> PRE-CREATION 
>   core/src/main/resources/oozie-default.xml b69d2c9 
>   core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21 
>   examples/src/main/apps/git/job.properties PRE-CREATION 
>   examples/src/main/apps/git/workflow.xml PRE-CREATION 
>   fluent-job/fluent-job-api/pom.xml 4c9b853 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java
>  7bb82e5 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java
>  PRE-CREATION 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java
>  ec56554 
>   fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e 
>   fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890 
>   pom.xml 92358aa 
>   sharelib/git/pom.xml PRE-CREATION 
>   sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java 
> PRE-CREATION 
>   
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java 
> PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.java 
> PRE-CREATION 
>   
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
>  PRE-CREATION 
>   sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java 
> PRE-CREATION 
>   
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
>  PRE-CREATION 
>   sharelib/pom.xml 6a0864d 
>   src/main/assemblies/sharelib.xml 07dc69c 
>   webapp/pom.xml fd3f89f 
> 
> 
> Diff: 

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

(Updated Aug. 24, 2018, 2:18 p.m.)


Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
Peter Bacsko.


Changes
---

Yet another review round.


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
8f0f244013932476d8ae454d224f235948529b34 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/6/

Changes: https://reviews.apache.org/r/68237/diff/5-6/


Testing
---

Tested on a real cluster, plus added test cases to 
`TestMapReduceActionExecutor` and new test classes.

Please note that I could find no proper way of actually getting MapReduce to 
start a new job while using `OozieClient#submit()` - apparently no YARN child 
application is created. Please advise what's the best way to advance, maybe 
call `JobClient#submitJob()` directly from 
`ActionExecutorTestCase#startWorkflowAndFailChildMRJob()`.


Thanks,

András Piros

Re: Review Request 68140: OOZIE-3229 Improved filtering options in V2SLAServlet

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 67-69 (patched)


Could be `static`



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 71 (patched)


Could be `static`



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 98 (patched)


I'd rather calculate `getColumnName()` as `this.name().toLowerCase()`.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 99-100 (patched)


Aren't `type` and `dbFieldName` somehow computable / readable via 
reflection from the actual `Entity` classes?

This would provide a single reason for change as in SRP, single point being 
only the `Entity` classes' field definitions.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 112-130 (patched)


Shouldn't we use the Criteria API to return the appropriate type (after 
conversion if needed) instead?



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 134-139 (patched)


Isn't Guava's `Lists#newArrayList(Iterables#filter(original, predicate))` a 
better option?



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 68-164 (original), 156-183 (patched)


I feel the need to extract the creation of the `CriteriaQuery` to another, 
maybe nested, class - for the sake of better testability.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Line 200 (original), 188 (patched)


You can use placeholder like `{0}` instead of `String` concatenation. 
Otherwise, big thumbs up :)



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Line 202 (original), 190 (patched)


We should list the possible `Exception` subclasses rather than one single 
general `catch` clause.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 214-217 (original), 196-200 (patched)


Needs better, more intuitive naming



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 202 (patched)


Needs better, more intuitive naming



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Line 245 (original), 220 (patched)


Needs better, more intuitive naming



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Line 330 (original), 238 (patched)


Needs better, more intuitive naming



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 246-249 (patched)


Extract magic `String`s to constant values



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 253-257 (patched)


Extract magic `String`s to constant values



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 259-261 (patched)


Extract magic `String`s to constant values



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 266-268 (patched)


Extract magic `String`s to constant values



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 273-275 (patched)


Extract magic `String`s to constant values



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 278 (patched)


Extract magic `String`s to constant values

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

(Updated Aug. 24, 2018, 12:17 p.m.)


Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
Peter Bacsko.


Changes
---

Addressing pre-commit build errors and Andras Salamon's review comments.


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
8f0f244013932476d8ae454d224f235948529b34 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/5/

Changes: https://reviews.apache.org/r/68237/diff/4-5/


Testing
---

Tested on a real cluster, plus added test cases to 
`TestMapReduceActionExecutor` and new test classes.

Please note that I could find no proper way of actually getting MapReduce to 
start a new job while using `OozieClient#submit()` - apparently no YARN child 
application is created. Please advise what's the best way to advance, maybe 
call `JobClient#submitJob()` directly from 
`ActionExecutorTestCase#startWorkflowAndFailChildMRJob()`.


Thanks,

András Piros

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

> On Aug. 24, 2018, 8:02 a.m., Andras Salamon wrote:
> > sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
> > Lines 1389 (patched)
> > 
> >
> > extra space indentation

This is because it's within a `try-with-resources` statement declaration.


- András


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


On Aug. 23, 2018, 6:36 p.m., András Piros wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68237/
> ---
> 
> (Updated Aug. 23, 2018, 6:36 p.m.)
> 
> 
> Review request for oozie, Andras Salamon, Attila Sasvari, Kinga Marton, and 
> Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
> treated as SUCCEEDED
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
> 8f0f244013932476d8ae454d224f235948529b34 
>   
> core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java
>  83a23f5220aa72ba15edc8b98ef80a74213fcee8 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
> f39bba2c691435354dac6da7794e5142b511d937 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
> a31079a41d30677d35a253a4a69505c21aa585f6 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
>  893405e38ad71d22460768b76ed03ac4d9a0b95d 
>   
> sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
>  f460b6bd11f60dfb397c6bba82be1427c2d1b570 
> 
> 
> Diff: https://reviews.apache.org/r/68237/diff/4/
> 
> 
> Testing
> ---
> 
> Tested on a real cluster, plus added test cases to 
> `TestMapReduceActionExecutor` and new test classes.
> 
> Please note that I could find no proper way of actually getting MapReduce to 
> start a new job while using `OozieClient#submit()` - apparently no YARN child 
> application is created. Please advise what's the best way to advance, maybe 
> call `JobClient#submitJob()` directly from 
> `ActionExecutorTestCase#startWorkflowAndFailChildMRJob()`.
> 
> 
> Thanks,
> 
> András Piros
> 
>

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

(Updated Aug. 23, 2018, 6:35 p.m.)


Review request for oozie, Attila Sasvari and Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
8f0f244013932476d8ae454d224f235948529b34 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdComparator.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/4/

Changes: https://reviews.apache.org/r/68237/diff/3-4/


Testing
---

Tested on a real cluster, plus added test cases to 
`TestMapReduceActionExecutor` and new test classes.

Please note that I could find no proper way of actually getting MapReduce to 
start a new job while using `OozieClient#submit()` - apparently no YARN child 
application is created. Please advise what's the best way to advance, maybe 
call `JobClient#submitJob()` directly from 
`ActionExecutorTestCase#startWorkflowAndFailChildMRJob()`.


Thanks,

András Piros

Re: Review Request 68482: OOZIE 3331 - SparkOptionsSplitter inconsistency

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Aug. 23, 2018, 7:04 a.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68482/
> ---
> 
> (Updated Aug. 23, 2018, 7:04 a.m.)
> 
> 
> Review request for oozie, András Piros and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE 3331 - SparkOptionsSplitter inconsistency
> 
> 
> Diffs
> -
> 
>   
> sharelib/spark/src/main/java/org/apache/oozie/action/hadoop/SparkOptionsSplitter.java
>  ac6ee81a 
>   
> sharelib/spark/src/test/java/org/apache/oozie/action/hadoop/TestSparkOptionsSplitter.java
>  61ded5ee 
> 
> 
> Diff: https://reviews.apache.org/r/68482/diff/1/
> 
> 
> Testing
> ---
> 
> TestSparkOptionsSplitter, TestSparkArgsExtractor unit tests
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 68411: OOZIE-3264 - Flaky test TestCoordMaterializeTransitionXCommand#testLastOnlyMaterialization

[András Piros via Review Board]

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




core/src/test/java/org/apache/oozie/command/coord/TestCoordMaterializeTransitionXCommand.java
Line 67 (original), 66 (patched)


`XTestCase` has its own `Services` field. Why do we need one more here?



core/src/test/java/org/apache/oozie/command/coord/TestCoordMaterializeTransitionXCommand.java
Lines 540-542 (patched)


Why do we need this at all? Why not rely on `tearDown()` instead?


- András Piros


On Aug. 17, 2018, 1:36 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68411/
> ---
> 
> (Updated Aug. 17, 2018, 1:36 p.m.)
> 
> 
> Review request for oozie, András Piros, Kinga Marton, and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-3264 - Flaky test 
> TestCoordMaterializeTransitionXCommand#testLastOnlyMaterialization
> 
> 
> Diffs
> -
> 
>   
> core/src/test/java/org/apache/oozie/command/coord/TestCoordChangeXCommand.java
>  b784093b 
>   
> core/src/test/java/org/apache/oozie/command/coord/TestCoordMaterializeTransitionXCommand.java
>  8189732e 
>   core/src/test/java/org/apache/oozie/test/XDataTestCase.java a8fe7037 
> 
> 
> Diff: https://reviews.apache.org/r/68411/diff/1/
> 
> 
> Testing
> ---
> 
> I was using grind. Before the modification it was failing 2 times out of 50 
> executions, after the patch it works 50/50.
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 68467: OOZIE-2684 Bad database schema error for WF_ACTIONS table

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/command/SchemaCheckXCommand.java
Lines 256 (patched)


Extract to local variable for better readability



core/src/main/java/org/apache/oozie/util/db/CompositeIndex.java
Lines 22-36 (patched)


Should be computed like:
```
String.format("CREATE INDEX I_%s_%s_%s ON %s (%s, %s", 
tableName.toUpperCase(), left.toUpperCase(), right.toUpperCase(), 
tableName.toUpperCase(), left.toUpperCase(), right.toUpperCase())
```

given the attributes `tableName`, `left`, and `right`.



core/src/main/java/org/apache/oozie/util/db/CompositeIndex.java
Lines 38 (patched)


Should be computed like discussed above.



core/src/main/java/org/apache/oozie/util/db/CompositeIndex.java
Lines 44-51 (patched)


Why not use `values()` instead?



core/src/main/java/org/apache/oozie/util/db/CompositeIndex.java
Lines 53 (patched)


Renamte to `find()`



core/src/test/java/org/apache/oozie/util/db/TestCompositeIndex.java
Lines 29 (patched)


Would assert with `String` constants instead of existing `enum` values.



core/src/test/java/org/apache/oozie/util/db/TestCompositeIndex.java
Lines 35 (patched)


Would assert with `String` constants instead of existing `enum` values.



tools/src/main/java/org/apache/oozie/tools/OozieDBCLI.java
Line 762 (original), 753 (patched)


Nice :)


- András Piros


On Aug. 22, 2018, 6:48 a.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68467/
> ---
> 
> (Updated Aug. 22, 2018, 6:48 a.m.)
> 
> 
> Review request for oozie, András Piros and Andras Salamon.
> 
> 
> Bugs: OOZIE-2684
> https://issues.apache.org/jira/browse/OOZIE-2684
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> In SchemaCheckerService, Oozie compares expected and found indexed columns 
> and generates the below error message which could be confusing to the users.
> 
> `2016-09-16 12:39:26,564  INFO SchemaCheckXCommand:520 - 
> SERVER[c6402.ambari.apache.org] USER[-] GROUP[-] TOKEN[-] APP[-] JOB[-] 
> ACTION[-] About to check database schema`
> `2016-09-16 12:39:26,703 ERROR SchemaCheckXCommand:517 - 
> SERVER[c6402.ambari.apache.org] USER[-] GROUP[-] TOKEN[-] APP[-] JOB[-] 
> ACTION[-] Found [1] extra indexes for columns in table [WF_ACTIONS]: [wf_id]`
> `2016-09-16 12:39:26,723 ERROR SchemaCheckXCommand:517 - 
> SERVER[c6402.ambari.apache.org] USER[-] GROUP[-] TOKEN[-] APP[-] JOB[-] 
> ACTION[-] Database schema is BAD! Check previous error log messages for 
> details`
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/command/SchemaCheckXCommand.java 
> 521408ee 
>   core/src/main/java/org/apache/oozie/util/db/CompositeIndex.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/oozie/util/db/TestCompositeIndex.java 
> PRE-CREATION 
>   tools/src/main/java/org/apache/oozie/tools/OozieDBCLI.java eeaaa60c 
> 
> 
> Diff: https://reviews.apache.org/r/68467/diff/1/
> 
> 
> Testing
> ---
> 
> Tested manually with MySql and PostgreSql.
> 
> I will open another Jira for making this part of code more testable, since 
> now is quite difficult to write some tests for it.
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 68438: OOZIE 3330 - [spark-action] Remove double quotes inside plain option values

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Aug. 21, 2018, 10:18 a.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68438/
> ---
> 
> (Updated Aug. 21, 2018, 10:18 a.m.)
> 
> 
> Review request for oozie, András Piros and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE 3330 - [spark-action] Remove double quotes inside plain option values
> 
> 
> Diffs
> -
> 
>   
> sharelib/spark/src/main/java/org/apache/oozie/action/hadoop/SparkOptionsSplitter.java
>  b6143223 
>   
> sharelib/spark/src/test/java/org/apache/oozie/action/hadoop/TestSparkArgsExtractor.java
>  60ab8b9f 
>   
> sharelib/spark/src/test/java/org/apache/oozie/action/hadoop/TestSparkOptionsSplitter.java
>  4be97772 
> 
> 
> Diff: https://reviews.apache.org/r/68438/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

(Updated Aug. 13, 2018, 3:39 p.m.)


Review request for oozie, Attila Sasvari and Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
8f0f244013932476d8ae454d224f235948529b34 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/hadoop/TestYarnApplicationIdFinder.java
 PRE-CREATION 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/3/

Changes: https://reviews.apache.org/r/68237/diff/2-3/


Testing
---

DO NOT MERGE YET.

Tested on a real cluster, plus added test cases to `TestJavaActionExecutor` and 
`TestMapReduceActionExecutor`.

Please note that I could find no proper way of actually getting MapReduce to 
start a new job while using `OozieClient#submit()` - apparently no YARN child 
application is created. Please advise what's the best way to advance, maybe 
call `JobClient#submitJob()` directly from 
`ActionExecutorTestCase#startWorkflowAndFailChildMRJob()`.


Thanks,

András Piros

Re: Review Request 67885: POC: OOZIE-3160 PriorityDelayQueue put()/take() can cause significant CPU load due to busy waiting

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java
Lines 143 (patched)


What about using a `CopyOnWriteArrayList` or a `ConcurrentHashSet` instead?



core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java
Lines 154 (patched)


Shouldn't we remove only when `executor.execute()` happened before?



core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java
Lines 163 (patched)


Here we rely on the fact that `commandFinished()` is called only once per 
task. If called multiple times, we're in a problem.



core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java
Lines 167 (patched)


Sure we need to give reference to `ThreadPoolExecutor` in a `public` way?



core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java
Lines 195 (patched)


`Integer#compareTo()` makes it more readable.



core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java
Lines 270 (patched)


`CopyOnWriteArrayList` or `ConcurrentHashSet` instead?



core/src/main/java/org/apache/oozie/service/CallableQueueService.java
Lines 122-126 (patched)


Dead code.



core/src/main/java/org/apache/oozie/service/CallableQueueService.java
Lines 165 (patched)


Unsure why we need an `AtomicInteger` here.



core/src/main/java/org/apache/oozie/service/CallableQueueService.java
Lines 182 (patched)


Unsure why we need an `AtomicInteger` here.



core/src/main/java/org/apache/oozie/service/CallableQueueService.java
Lines 559 (patched)


`LOG.debug()` on which implementation is running would be nice.



core/src/main/java/org/apache/oozie/util/PriorityDelayQueue.java
Line 67 (original), 67-68 (patched)


A javadoc about the difference between the semantics of `delay` and 
`initialDelay` would be nice.



core/src/test/java/org/apache/oozie/service/TestAsyncXCommandExecutor.java
Lines 103-111 (patched)


Duplicate.



core/src/test/java/org/apache/oozie/service/TestAsyncXCommandExecutor.java
Lines 155-163 (patched)


Duplicate.



core/src/test/java/org/apache/oozie/service/TestAsyncXCommandExecutor.java
Lines 207 (patched)


Are there 4 priority levels, from 0 to 3?



core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java
Lines 412-415 (original), 427-440 (patched)


Dead code.



core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java
Line 425 (original), 455 (patched)


Dead code.



core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java
Lines 1075 (patched)


Better use `taskCount / 3`.


- András Piros


On Aug. 6, 2018, 3:28 p.m., Peter Bacsko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67885/
> ---
> 
> (Updated Aug. 6, 2018, 3:28 p.m.)
> 
> 
> Review request for oozie, András Piros, Peter Cseh, Kinga Marton, and Robert 
> Kanter.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Still just a POC.
> 
> Tests are almost completely missing.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/service/AsyncXCommandExecutor.java 
> PRE-CREATION 
>   core/src/main/java/org/apache/oozie/service/CallableAccess.java 
> PRE-CREATION 
>   core/src/main/java/org/apache/oozie/service/CallableQueueService.java 
> ef8d58da5 
>   core/src/main/java/org/apache/oozie/util/PriorityDelayQueue.java 75c20698c 
>   core/src/test/java/org/apache/oozie/service/TestAsyncXCommandExecutor.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java 
> 9c2a11d6f 
> 
> 
> Diff: https://reviews.apache.org/r/67885/diff/5/
> 
> 
> Testing
> ---
> 
> Executed TestCallableQueueService which passed completely.
> 
> 
> Thanks,
> 
> Peter Bacsko
> 
>

Re: Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

(Updated Aug. 10, 2018, 10:52 a.m.)


Review request for oozie, Attila Sasvari and Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
8f0f244013932476d8ae454d224f235948529b34 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/2/

Changes: https://reviews.apache.org/r/68237/diff/1-2/


Testing
---

DO NOT MERGE YET.

Tested on a real cluster, plus added test cases to `TestJavaActionExecutor` and 
`TestMapReduceActionExecutor`.

Please note that I could find no proper way of actually getting MapReduce to 
start a new job while using `OozieClient#submit()` - apparently no YARN child 
application is created. Please advise what's the best way to advance, maybe 
call `JobClient#submitJob()` directly from 
`ActionExecutorTestCase#startWorkflowAndFailChildMRJob()`.


Thanks,

András Piros

Re: Review Request 67999: Parsing sharelib timestamps is not threadsafe

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Aug. 9, 2018, 8:11 a.m., Denes Bodo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67999/
> ---
> 
> (Updated Aug. 9, 2018, 8:11 a.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Bugs: OOZIE-3304
> https://issues.apache.org/jira/browse/OOZIE-3304
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> In rare cases the fNumberFormatException occurs while parsing latest sharelib 
> directory names
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/service/ShareLibService.java a901567 
>   core/src/test/java/org/apache/oozie/service/TestHAShareLibService.java 
> 5087505 
>   core/src/test/java/org/apache/oozie/service/TestShareLibService.java 
> d244166 
> 
> 
> Diff: https://reviews.apache.org/r/67999/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Denes Bodo
> 
>

Re: Review Request 68255: OOZIE 3314 - Remove findbugs-filter.xml and convert its contents to annotations

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Aug. 8, 2018, 9:15 a.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68255/
> ---
> 
> (Updated Aug. 8, 2018, 9:15 a.m.)
> 
> 
> Review request for oozie, András Piros and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> 3 excptions replaced by annotations, the 4th one affects generated code, 
> still using a (simpified) XML for that.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/command/XCommand.java 7b8f47cc 
>   core/src/main/java/org/apache/oozie/command/coord/CoordinatorXCommand.java 
> 18937a4b 
>   core/src/main/java/org/apache/oozie/command/wf/WorkflowXCommand.java 
> 87d7e77f 
>   core/src/main/java/org/apache/oozie/util/Instrumentation.java 8bcb64c1 
>   findbugs-filter.xml 133178f0 
>   fluent-job/fluent-job-api/pom.xml 4c9b8533 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/GraphVisualization.java
>  437d6b3b 
>   fluent-job/pom.xml 5b24c911 
>   pom.xml 92358aa2 
> 
> 
> Diff: https://reviews.apache.org/r/68255/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Re: Review Request 68255: OOZIE 3314 - Remove findbugs-filter.xml and convert its contents to annotations

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/command/XCommand.java
Lines 84 (patched)


Typo: eventService



core/src/main/java/org/apache/oozie/command/XCommand.java
Line 82 (original), 85 (patched)


Actually `eventService` is only written from inside one of the 
constructors, so it could - and should - be `static final`. Maybe can also be 
`private` and provide a getter for the ones wanting to reach it, or at least a 
`@VisibleForTesting`, when that's the case.



core/src/main/java/org/apache/oozie/util/Instrumentation.java
Lines 710-711 (patched)


Any other way to refactor?


- András Piros


On Aug. 7, 2018, 12:54 p.m., Andras Salamon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68255/
> ---
> 
> (Updated Aug. 7, 2018, 12:54 p.m.)
> 
> 
> Review request for oozie, András Piros and Kinga Marton.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> 3 excptions replaced by annotations, the 4th one affects generated code, 
> still using a (simpified) XML for that.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/command/XCommand.java 7b8f47cc 
>   core/src/main/java/org/apache/oozie/util/Instrumentation.java 8bcb64c1 
>   findbugs-filter.xml 133178f0 
>   fluent-job/fluent-job-api/pom.xml 4c9b8533 
>   
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/GraphVisualization.java
>  437d6b3b 
>   fluent-job/pom.xml 5b24c911 
>   pom.xml 92358aa2 
> 
> 
> Diff: https://reviews.apache.org/r/68255/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andras Salamon
> 
>

Review Request 68237: OOZIE-3298 OYA: external ID is not filled properly and failing MR job is treated as SUCCEEDED

[András Piros via Review Board]

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

Review request for oozie, Attila Sasvari and Peter Bacsko.


Repository: oozie-git


Description
---

OOZIE-3298 OYA: external ID is not filled properly and failing MR job is 
treated as SUCCEEDED


Diffs
-

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
8f0f244013932476d8ae454d224f235948529b34 
  
core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java 
83a23f5220aa72ba15edc8b98ef80a74213fcee8 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
f39bba2c691435354dac6da7794e5142b511d937 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
a31079a41d30677d35a253a4a69505c21aa585f6 
  
core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
 893405e38ad71d22460768b76ed03ac4d9a0b95d 
  
sharelib/streaming/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java
 f460b6bd11f60dfb397c6bba82be1427c2d1b570 


Diff: https://reviews.apache.org/r/68237/diff/1/


Testing
---

DO NOT MERGE YET.

Tested on a real cluster, plus added test cases to `TestJavaActionExecutor` and 
`TestMapReduceActionExecutor`.

Please note that I could find no proper way of actually getting MapReduce to 
start a new job while using `OozieClient#submit()` - apparently no YARN child 
application is created. Please advise what's the best way to advance, maybe 
call `JobClient#submitJob()` directly from 
`ActionExecutorTestCase#startWorkflowAndFailChildMRJob()`.


Thanks,

András Piros

Re: Review Request 68180: OOZIE-3315 Datelist Java Main example fails

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Aug. 6, 2018, 12:21 p.m., Daniel Becker wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68180/
> ---
> 
> (Updated Aug. 6, 2018, 12:21 p.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> The oozie/examples/src/main/java/org/apache/oozie/example/DateList.java 
> produces a wrong action output:
> 
> ,2009-02-01T01:15Z,2009-02-01T01:30Z,2009-02-01T01:45Z,2009-02-01T02:00Z
> instead of
> 
> 2009-02-01T01:00Z,2009-02-01T01:15Z,2009-02-01T01:30Z,2009-02-01T01:45Z
> the first element is missing in the list (but not the separator). This is 
> caused by an off-by-one error introduced in OOZIE-2942.
> 
> 
> Diffs
> -
> 
>   examples/src/main/java/org/apache/oozie/example/DateList.java 731fe413 
>   examples/src/test/java/org/apache/oozie/example/TestDateList.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68180/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Becker
> 
>

Re: Review Request 68140: OOZIE-3229 Improved filtering options in V2SLAServlet

[András Piros via Review Board]

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




client/src/main/java/org/apache/oozie/client/OozieClient.java
Lines 177-204 (patched)


Can you please structure that a bit more? Like having not `String` but 
`enum` instances, having a nested wrapper class...



core/src/main/conf/action-conf/hive.xml
Line 28 (original), 28 (patched)


Please revert this change.



core/src/main/java/org/apache/oozie/action/ActionExecutor.java
Line 606 (original), 606 (patched)


Please revert this change.



core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Line 1780 (original), 1780 (patched)


Please revert this change.



core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1831-1834 (original)


Please revert this change.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 51-78 (patched)


Can we extract that to a state object? Too many fields here, we're far away 
from Single Responsibility Principle.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Line 66 (original), 91 (patched)


Can we please apply some kind of object-oriented design pattern, like 
Builder, here? One huge builder with multiple child builders.

I feel that too many `if-then-else` and too long methods are disturbing 
beyond readability / comprehension.

I also would separate out different responsibilities to different, maybe 
nested, classes - and test them separately.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 267-277 (original), 294-311 (patched)


Please extract method.



core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java
Lines 425-426 (patched)


Here we're changing existing behavior. Any chance to create separate test 
for the change, plus document that?



core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java
Lines 69-78 (original), 68-94 (patched)


This looks pretty terrible, please extract / restructure for better 
maintainability.



core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java
Line 115 (original), 131 (patched)


Can we please apply some kind of object-oriented design pattern, like 
Builder, here? One huge builder with multiple child builders.

I feel that too many `if-then-else` and too long methods are disturbing 
beyond readability / comprehension.

I also would separate out different responsibilities to different, maybe 
nested, classes - and test them separately.



core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java
Lines 136-141 (original), 153-175 (patched)


This looks pretty terrible, again. What about applying some design pattern 
/ restructuring the code?



core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java
Lines 144-147 (original), 178-198 (patched)


This looks pretty terrible, again. What about applying some design pattern 
/ restructuring the code?



core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java
Line 16 (original), 16 (patched)


Please revert this change.



core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
Line 19 (original), 19 (patched)


Please revert this change.



core/src/test/java/org/apache/oozie/servlet/TestV2SLAServlet.java
Lines 111-115 (patched)


Please extract to separate test methods to provide an even more 
maintainable piece of code :)



docs/src/site/twiki/DG_SLAMonitoring.twiki
Lines 173-182 (patched)


We'd need also `curl` based examples with sample JSON output for better 
user insight.



release-log.txt
Lines 3-4 (original)


Please revert this change.



webapp/src/main/webapp/console/sla/js/oozie-sla.js
Line 29 (original), 29 (patched)

Re: Review Request 68180: OOZIE-3315 Datelist Java Main example fails

[András Piros via Review Board]

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




examples/src/main/java/org/apache/oozie/example/DateList.java
Lines 43-47 (patched)


Could be extracted to `createDateList()`.



examples/src/test/java/org/apache/oozie/example/TestDateList.java
Lines 63-64 (patched)


Please use telling assertion error messages.



examples/src/test/java/org/apache/oozie/example/TestDateList.java
Lines 76 (patched)


Please remove.



examples/src/test/java/org/apache/oozie/example/TestDateList.java
Lines 87 (patched)


Please use telling assertion error messages.


- András Piros


On Aug. 6, 2018, 8:57 a.m., Daniel Becker wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68180/
> ---
> 
> (Updated Aug. 6, 2018, 8:57 a.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> The oozie/examples/src/main/java/org/apache/oozie/example/DateList.java 
> produces a wrong action output:
> 
> ,2009-02-01T01:15Z,2009-02-01T01:30Z,2009-02-01T01:45Z,2009-02-01T02:00Z
> instead of
> 
> 2009-02-01T01:00Z,2009-02-01T01:15Z,2009-02-01T01:30Z,2009-02-01T01:45Z
> the first element is missing in the list (but not the separator). This is 
> caused by an off-by-one error introduced in OOZIE-2942.
> 
> 
> Diffs
> -
> 
>   examples/src/main/java/org/apache/oozie/example/DateList.java 731fe413 
>   examples/src/test/java/org/apache/oozie/example/TestDateList.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68180/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Becker
> 
>

Re: Review Request 68180: OOZIE-3315 Datelist Java Main example fails

[András Piros via Review Board]

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




examples/pom.xml
Lines 108-112 (patched)


Can you please look whether `com.github.stefanbirkner:system-rules` [CPL 
1.0](https://github.com/stefanbirkner/system-rules/blob/master/LICENSE) is 
compatible with Apache 2.0?



examples/src/test/java/org/apache/oozie/example/TestDateList.java
Lines 1 (patched)


Apache 2.0 license header missing.



examples/src/test/java/org/apache/oozie/example/TestDateList.java
Lines 27 (patched)


Neat :)


- András Piros


On Aug. 3, 2018, 6:38 a.m., Daniel Becker wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68180/
> ---
> 
> (Updated Aug. 3, 2018, 6:38 a.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> The oozie/examples/src/main/java/org/apache/oozie/example/DateList.java 
> produces a wrong action output:
> 
> ,2009-02-01T01:15Z,2009-02-01T01:30Z,2009-02-01T01:45Z,2009-02-01T02:00Z
> instead of
> 
> 2009-02-01T01:00Z,2009-02-01T01:15Z,2009-02-01T01:30Z,2009-02-01T01:45Z
> the first element is missing in the list (but not the separator). This is 
> caused by an off-by-one error introduced in OOZIE-2942.
> 
> 
> Diffs
> -
> 
>   examples/pom.xml 680e3fb1 
>   examples/src/main/java/org/apache/oozie/example/DateList.java 731fe413 
>   examples/src/test/java/org/apache/oozie/example/TestDateList.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68180/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Becker
> 
>

Re: Review Request 68102: OOZIE-3193 Applications are not killed when submitted via subworkflow

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Aug. 1, 2018, 1:44 p.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68102/
> ---
> 
> (Updated Aug. 1, 2018, 1:44 p.m.)
> 
> 
> Review request for oozie, András Piros and Peter Cseh.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Tags were not generated properly for external child jobs launched from subWFs 
> and they were not killed.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/action/ActionExecutor.java 71bd36ae6 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
> a1a9671ab 
>   core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java 
> fdca5706e 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/SleepMapperReducerForTest.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
>  e074d482f 
> 
> 
> Diff: https://reviews.apache.org/r/68102/diff/3/
> 
> 
> Testing
> ---
> 
> Tested manually.
> Added Junit test.
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 68102: OOZIE-3193 Applications are not killed when submitted via subworkflow

[András Piros via Review Board]

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




core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
Lines 931-932 (patched)


Can be `final`.



core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
Lines 946-947 (patched)


Can be `final`.


- András Piros


On Aug. 1, 2018, 9:59 a.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68102/
> ---
> 
> (Updated Aug. 1, 2018, 9:59 a.m.)
> 
> 
> Review request for oozie, András Piros and Peter Cseh.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Tags were not generated properly for external child jobs launched from subWFs 
> and they were not killed.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/action/ActionExecutor.java 71bd36ae6 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
> a1a9671ab 
>   core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java 
> fdca5706e 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/SleepMapperReducerForTest.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
>  e074d482f 
> 
> 
> Diff: https://reviews.apache.org/r/68102/diff/2/
> 
> 
> Testing
> ---
> 
> Tested manually.
> Added Junit test.
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 68102: OOZIE-3193 Applications are not killed when submitted via subworkflow

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On Aug. 1, 2018, 9:59 a.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68102/
> ---
> 
> (Updated Aug. 1, 2018, 9:59 a.m.)
> 
> 
> Review request for oozie, András Piros and Peter Cseh.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Tags were not generated properly for external child jobs launched from subWFs 
> and they were not killed.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/action/ActionExecutor.java 71bd36ae6 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
> a1a9671ab 
>   core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java 
> fdca5706e 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/SleepMapperReducerForTest.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
>  e074d482f 
> 
> 
> Diff: https://reviews.apache.org/r/68102/diff/2/
> 
> 
> Testing
> ---
> 
> Tested manually.
> Added Junit test.
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 67999: Parsing sharelib timestamps is not threadsafe

[András Piros via Review Board]

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




core/src/test/java/org/apache/oozie/service/TestShareLibService.java
Lines 1052 (patched)


Nice catch!


- András Piros


On July 20, 2018, 11 a.m., Denes Bodo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67999/
> ---
> 
> (Updated July 20, 2018, 11 a.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Bugs: OOZIE-3304
> https://issues.apache.org/jira/browse/OOZIE-3304
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> In rare cases the fNumberFormatException occurs while parsing latest sharelib 
> directory names
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/service/ShareLibService.java a901567 
>   core/src/test/java/org/apache/oozie/service/TestHAShareLibService.java 
> 5087505 
>   core/src/test/java/org/apache/oozie/service/TestShareLibService.java 
> d244166 
> 
> 
> Diff: https://reviews.apache.org/r/67999/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Denes Bodo
> 
>

Re: Review Request 68008: OOZIE-2942 [examples] Fix Findbugs warnings

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On July 22, 2018, 1:16 p.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68008/
> ---
> 
> (Updated July 22, 2018, 1:16 p.m.)
> 
> 
> Review request for oozie, András Piros and Peter Cseh.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Currently Findbugs complains about the following warnings in the 
> oozie-examples module:
> ```
> [INFO] org.apache.oozie.example.DateList.main(String[]) may fail to close 
> stream on exception [org.apache.oozie.example.DateList] At 
> DateList.java:[line 69]
> [INFO] Nullcheck of date at line 55 of value previously dereferenced in 
> org.apache.oozie.example.DateList.main(String[]) 
> [org.apache.oozie.example.DateList, org.apache.oozie.example.DateList] At 
> DateList.java:[line 55]Redundant null check at DateList.java:[line 62]
> [INFO] Private method 
> org.apache.oozie.example.DateList.formatDateUTC(Calendar) is never called 
> [org.apache.oozie.example.DateList] At DateList.java:[line 97]
> [INFO] org.apache.oozie.example.LocalOozieExample.execute(String[]) may fail 
> to clean up java.io.InputStream [org.apache.oozie.example.LocalOozieExample, 
> org.apache.oozie.example.LocalOozieExample, 
> org.apache.oozie.example.LocalOozieExample, 
> org.apache.oozie.example.LocalOozieExample, 
> org.apache.oozie.example.LocalOozieExample, 
> org.apache.oozie.example.LocalOozieExample, 
> org.apache.oozie.example.LocalOozieExample, 
> org.apache.oozie.example.LocalOozieExample, 
> org.apache.oozie.example.LocalOozieExample] Obligation to clean up resource 
> created at LocalOozieExample.java:[line 72] is not dischargedPath continues 
> at LocalOozieExample.java:[line 76]Path continues at 
> LocalOozieExample.java:[line 77]Path continues at 
> LocalOozieExample.java:[line 78]Path continues at 
> LocalOozieExample.java:[line 81]Path continues at 
> LocalOozieExample.java:[line 88]Path continues at 
> LocalOozieExample.java:[line 89]Path continues at 
> LocalOozieExample.java:[line 91]Path continues at 
> LocalOozieExample.java:[line 
 100]
> [INFO] org.apache.oozie.example.LocalOozieExample.execute(String[]) may fail 
> to close stream [org.apache.oozie.example.LocalOozieExample] At 
> LocalOozieExample.java:[line 72]
> [INFO] org.apache.oozie.example.Repeatable.getBaseline() may expose internal 
> representation by returning Repeatable.baseline 
> [org.apache.oozie.example.Repeatable] At Repeatable.java:[line 168]
> [INFO] org.apache.oozie.example.Repeatable.setBaseline(Date) may expose 
> internal representation by storing an externally mutable object into 
> Repeatable.baseline [org.apache.oozie.example.Repeatable] At 
> Repeatable.java:[line 172]
> ```
> 
> They should be fixed to get the code more reliable.
> 
> 
> Diffs
> -
> 
>   examples/src/main/java/org/apache/oozie/example/DateList.java 7e574cbe 
>   examples/src/main/java/org/apache/oozie/example/LocalOozieExample.java 
> c9f5697c 
>   examples/src/main/java/org/apache/oozie/example/Repeatable.java ee863251 
> 
> 
> Diff: https://reviews.apache.org/r/68008/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept multiple source folders

[András Piros via Review Board]

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




tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
Lines 46 (patched)


Shouldn't it be rather `@Rule public TemporaryFolder tmpFolder = new 
TemporaryFolder()`?



tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
Lines 82 (patched)


Please provide more context to the assertion error message.



tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
Lines 102 (patched)


Please provide more context to the assertion error message.



tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
Lines 148 (patched)


Please provide more context to the assertion error message.



tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 81-82 (original), 80-81 (patched)


Please provide more context to the assertion error message.



tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Line 169 (original), 103 (patched)


Please provide more context to the assertion error message.


- András Piros


On July 25, 2018, 8:48 a.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67834/
> ---
> 
> (Updated July 25, 2018, 8:48 a.m.)
> 
> 
> Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Right now sharelib can be created via sharelib create -fs FS_URI -locallib 
> SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
> It would be nice to have the possibility to define additional folders to be 
> uploaded into the sharelib, so the users don't have to copy or link the files 
> together on their machine.
> The syntax could be something like -additional-lib 
> sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
> 
> 
> Diffs
> -
> 
>   docs/src/site/twiki/AG_Install.twiki 46363a3b3 
>   tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02 
>   
> tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
>  PRE-CREATION 
>   tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java 
> d344300ed 
>   tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java 
> d77eba69d 
>   tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java 
> bce0433c6 
>   tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 
> 5929e5ca0 
>   
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIExtraArgsParser.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67834/diff/7/
> 
> 
> Testing
> ---
> 
> Tested manually + added integration test
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 68102: OOZIE-3193 Applications are not killed when submitted via subworkflow

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1781-1782 (patched)


Please remove code comment and extract to a well-named method instead.



core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java
Lines 106-121 (patched)


Extract method.



core/src/test/java/org/apache/oozie/action/hadoop/SleepMapperReducerForTest.java
Lines 32 (patched)


`SLEEP_TIME_MILLIS_KEY` is a better name.



core/src/test/java/org/apache/oozie/action/hadoop/SleepMapperReducerForTest.java
Lines 60-61 (patched)


Isn't emitting another message also just after sleep a good idea?



core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
Lines 544 (patched)


Please remove white space between method name and brackets.



core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
Lines 555-574 (patched)


Please extract to `private static class ApplicationIdExistsPredicate 
implements Predicate`.



core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
Lines 581-588 (patched)


Please extract to `private static class SubWorkflowActionRunningPredicate 
implements Predicate`.



core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
Lines 708-712 (patched)


Please use latest schema version `1.0`.


- András Piros


On July 30, 2018, 8:42 a.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68102/
> ---
> 
> (Updated July 30, 2018, 8:42 a.m.)
> 
> 
> Review request for oozie, András Piros and Peter Cseh.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Tags were not generated properly for external child jobs launched from subWFs 
> and they were not killed.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/action/ActionExecutor.java 71bd36ae6 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
> a1a9671ab 
>   core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java 
> fdca5706e 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/SleepMapperReducerForTest.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java
>  e074d482f 
> 
> 
> Diff: https://reviews.apache.org/r/68102/diff/1/
> 
> 
> Testing
> ---
> 
> Tested manually.
> Added Junit test.
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept multiple source folders

[András Piros via Review Board]

> On July 6, 2018, 1:40 p.m., András Piros wrote:
> > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
> > Lines 88 (patched)
> > 
> >
> > Problem is we can have ```hdfs://my/jar.jar#myjar.jar``` inside 
> > sharelib paths.
> > 
> > Best is to go with e.g. `;` that cannot be present as HDFS or local 
> > file.
> 
> Kinga Marton wrote:
> Using semicolon(;) is not a good ideea because the shell handles it as a 
> command separator. What about using & ?

I'm OK using colon `,` as the separator between sharelib name-path pairs, and 
pipe `|` between sharelib paths like this:
```
additional-lib 
sharelibName1=/path/to/source/|/path/to/some/file,sharelibName2=/path/to/some/folder
```


- András


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


On July 6, 2018, 8:36 a.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67834/
> ---
> 
> (Updated July 6, 2018, 8:36 a.m.)
> 
> 
> Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Right now sharelib can be created via sharelib create -fs FS_URI -locallib 
> SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
> It would be nice to have the possibility to define additional folders to be 
> uploaded into the sharelib, so the users don't have to copy or link the files 
> together on their machine.
> The syntax could be something like -additional-lib 
> sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
> 
> 
> Diffs
> -
> 
>   docs/src/site/twiki/AG_Install.twiki 46363a3b3 
>   tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02 
>   tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java 
> d344300ed 
>   tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java 
> d77eba69d 
>   tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java 
> bce0433c6 
>   tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 
> 5929e5ca0 
>   
> tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67834/diff/4/
> 
> 
> Testing
> ---
> 
> Tested manually + added integration test
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept multiple source folders

[András Piros via Review Board]

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




docs/src/site/twiki/AG_Install.twiki
Lines 60 (patched)


Typo: comma.



docs/src/site/twiki/AG_Install.twiki
Lines 61 (patched)


Would be better to use mixed separators as per functionality:
```

sharelib-name-1=path-name-1,path-name-2:sharelib-name-2=path-name-1,path-name-2
```



tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 74-86 (patched)


Extracting `System.lineSeparator()` to a constant field would increase 
readability.



tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 88 (patched)


Problem is we can have ```hdfs://my/jar.jar#myjar.jar``` inside sharelib 
paths.

Best is to go with e.g. `;` that cannot be present as HDFS or local file.



tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 264 (patched)


Shouldn't it be `System.out`?



tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 282-294 (patched)


A log message about how parallel we are running would be nice.



tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java
Lines 42-48 (original), 43-51 (patched)


Could be extracted to a nested `static class`, and tested separately.



tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 181-182 (original), 112-113 (patched)


Multiple testing scenarios missing:

* empty value to a key present
* multiple values to a key
* value without a key

Please use a separate test file.



tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 116-118 (patched)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 143 (patched)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 152 (patched)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 84 (patched)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 104 (patched)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 130 (patched)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 147 (patched)


Nice `message` :)


- András Piros


On July 6, 2018, 8:36 a.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67834/
> ---
> 
> (Updated July 6, 2018, 8:36 a.m.)
> 
> 
> Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Right now sharelib can be created via sharelib create -fs FS_URI -locallib 
> SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
> It would be nice to have the possibility to define additional folders to be 
> uploaded into the sharelib, so the users don't have to copy or link the files 
> together on their machine.
> The syntax could be something like -additional-lib 
> sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
> 
> 
> Diffs
> -
> 
>   docs/src/site/twiki/AG_Install.twiki 46363a3b3 
>   tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02 
>   tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java 
> d344300ed 
>   tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java 
> d77eba69d 
>   tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java 
> bce0433c6 
>   tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 
> 5929e5ca0 
>   
> tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
>  PRE-CREATION 
> 
> 
> Diff: 

Re: Review Request 66656: Exclusion pattern for sharelib.

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1119-1120 (patched)


Do a multi-catch instead.

Would also re-throw some kind of `IllegalStateException` with appropriate 
error message, but by no means a `NullPointerException` down the code path 
without any meaning.


- András Piros


On July 3, 2018, 1:30 p.m., Mate Juhasz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66656/
> ---
> 
> (Updated July 3, 2018, 1:30 p.m.)
> 
> 
> Review request for oozie, András Piros and Denes Bodo.
> 
> 
> Bugs: OOZIE-1624
> https://issues.apache.org/jira/browse/OOZIE-1624
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-1624 Exclusion pattern for sharelib.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 
> ed809ef0 
>   core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java 
> PRE-CREATION 
>   core/src/main/java/org/apache/oozie/service/ShareLibService.java a901567d 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 
> f39bba2c 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 
> a31079a4 
>   
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
>  PRE-CREATION 
>   core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java 
> PRE-CREATION 
>   docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21e 
> 
> 
> Diff: https://reviews.apache.org/r/66656/diff/4/
> 
> 
> Testing
> ---
> 
> Tested on a cluster
> 
> 
> Thanks,
> 
> Mate Juhasz
> 
>

Re: Review Request 66656: Exclusion pattern for sharelib.

[András Piros via Review Board]

> On June 20, 2018, 10:56 a.m., András Piros wrote:
> > core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
> > Lines 433-441 (patched)
> > 
> >
> > Extract method `checkFilesInClassPath(final Path[] baseClasspath, final 
> > Path[] filesToCheck, final Predicate assertion)`
> 
> Mate Juhasz wrote:
> Could you help me with an example how to use 'Predicate 
> assertion' in this case?
> 
> András Piros wrote:
> It's even simpler than that :) a `boolean` would suffice.
> 
> Here is the complete refactor:
> ```
> 
> private void verifyFilesInDistributedCache(Map libs, 
> Configuration jobConf) throws IOException {
> assertTrue(DistributedCache.getSymlink(jobConf));
> 
> Path[] baseClassPath = {new Path(getAppPath(), TEST_JAR), 
> libs.get(TEST_ROOT_JAR)};
> Path[] filesToCheck = DistributedCache.getFileClassPaths(jobConf);
> boolean shouldBePresent = true;
> checkFilesInClasspath(baseClassPath, filesToCheck, 
> shouldBePresent);
> 
> baseClassPath = new Path[]{new Path(getAppPath(), TEST_FILE), 
> libs.get(TEST_ROOT_FILE),
> new Path(getAppPath(), TEST_FILE_SO), 
> libs.get(TEST_ROOT_SO),
> new Path(getAppPath(), TEST_FILE_SO_1), 
> libs.get(TEST_ROOT_SO_1)};
> shouldBePresent = false;
> checkFilesInClasspath(baseClassPath, filesToCheck, 
> shouldBePresent);
> 
> baseClassPath = new Path[]{new Path(getAppPath(), TEST_JAR), 
> libs.get(TEST_ROOT_JAR),
> new Path(getAppPath(), TEST_FILE), 
> libs.get(TEST_ROOT_FILE),
> new Path(getAppPath(), TEST_FILE_SO), 
> libs.get(TEST_ROOT_SO),
> new Path(getAppPath(), TEST_FILE_SO_1), 
> libs.get(TEST_ROOT_SO_1)};
> filesToCheck = 
> urisToPaths(DistributedCache.getCacheFiles(jobConf));
> shouldBePresent = true;
> checkFilesInClasspath(baseClassPath, filesToCheck, 
> shouldBePresent);
> 
> baseClassPath = new Path[]{new Path(getAppPath(), TEST_ARCHIVE), 
> libs.get(TEST_ROOT_ARCHIVE)};
> filesToCheck = 
> urisToPaths(DistributedCache.getCacheArchives(jobConf));
> checkFilesInClasspath(baseClassPath, filesToCheck, 
> shouldBePresent);
> }
> 
> private void checkFilesInClasspath(final Path[] baseClasspath, final 
> Path[] filesToCheck, final boolean shouldBePresent) {
> for (final Path basePath : baseClasspath) {
> boolean found = false;
> for (final Path fileToCheck : filesToCheck) {
> if (!found && 
> basePath.toUri().getPath().equals(fileToCheck.toUri().getPath())) {
> found = true;
> }
> }
> if (shouldBePresent) {
> assertTrue("file " + basePath.toUri().getPath() + " not 
> found in classpath", found);
> }
> else {
> assertFalse("file " + basePath.toUri().getPath() + " 
> found in classpath", found);
> }
> }
> 
> }
> ```
> 
> Isn't it way nicer? ;)

Oh sorry, the complete refactor again:

```
private void verifyFilesInDistributedCache(Map libs, 
Configuration jobConf) throws IOException {
assertTrue(DistributedCache.getSymlink(jobConf));

Path[] baseClassPath = {new Path(getAppPath(), TEST_JAR), 
libs.get(TEST_ROOT_JAR)};
Path[] filesToCheck = DistributedCache.getFileClassPaths(jobConf);
boolean shouldBePresent = true;
checkFilesInClasspath(baseClassPath, filesToCheck, shouldBePresent);

baseClassPath = new Path[]{new Path(getAppPath(), TEST_FILE), 
libs.get(TEST_ROOT_FILE),
new Path(getAppPath(), TEST_FILE_SO), libs.get(TEST_ROOT_SO),
new Path(getAppPath(), TEST_FILE_SO_1), 
libs.get(TEST_ROOT_SO_1)};
shouldBePresent = false;
checkFilesInClasspath(baseClassPath, filesToCheck, shouldBePresent);

baseClassPath = new Path[]{new Path(getAppPath(), TEST_JAR), 
libs.get(TEST_ROOT_JAR),
new Path(getAppPath(), TEST_FILE), libs.get(TEST_ROOT_FILE),
new Path(getAppPath(), TEST_FILE_SO), libs.get(TEST_ROOT_SO),
new Path(getAppPath(), TEST_FILE_SO_1), 
libs.get(TEST_ROOT_SO_1)};
filesToCheck = urisToPaths(DistributedCache.getCacheFiles(jobConf));
shouldBePresent = true;
checkFilesInClasspath(baseClassPath, filesToCheck, shouldBePresent);

baseClassPath = new Path[]{new Path(getAppPath(), TEST_ARCHIVE), 
libs.get(TEST_ROOT_ARCHIVE)};
filesToCheck = urisToPaths(DistributedCache.getCacheArchives(jobConf));
checkFilesInClasspath(baseClassPath, filesToCheck, 

Re: Review Request 67766: OOZIE-2955 Fix Findbugs warnings related to reliance on default encoding in oozie-client

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On July 2, 2018, 8:06 a.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67766/
> ---
> 
> (Updated July 2, 2018, 8:06 a.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Currently Findbugs complains about some warnings related to the reliance on 
> the default encoding in the oozie-client module:
> They should be fixed to get the code more reliable.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/cli/CLIParser.java bbdb80356 
>   client/src/main/java/org/apache/oozie/cli/OozieCLI.java bc234e318 
>   client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 98f421f32 
>   client/src/main/java/org/apache/oozie/client/OozieClient.java 2cc169230 
>   client/src/main/java/org/apache/oozie/client/XOozieClient.java ba60f8e75 
> 
> 
> Diff: https://reviews.apache.org/r/67766/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 67784: Escape log-streaming's HTML-specific characters

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On June 29, 2018, 12:28 p.m., Denes Bodo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67784/
> ---
> 
> (Updated June 29, 2018, 12:28 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-3109
> https://issues.apache.org/jira/browse/OOZIE-3109
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> That can be a security problem if we do not encode the stream read from log 
> files before transferred to the web browser.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/service/XLogStreamingService.java 
> 3cfbeac 
>   core/src/main/java/org/apache/oozie/service/ZKXLogStreamingService.java 
> 3a5081c 
>   core/src/main/java/org/apache/oozie/util/TimestampedMessageParser.java 
> 1b87605 
>   core/src/main/java/org/apache/oozie/util/XLogStreamer.java f0291af 
>   core/src/test/java/org/apache/oozie/service/TestXLogStreamingService.java 
> 1921f1b 
> 
> 
> Diff: https://reviews.apache.org/r/67784/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Denes Bodo
> 
>

Re: Review Request 67763: OOZIE-2956 Fix Findbugs warnings related to reliance on default encoding in oozie-core

[András Piros via Review Board]

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


Ship it!




Ship It!

- András Piros


On July 1, 2018, 10:47 a.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67763/
> ---
> 
> (Updated July 1, 2018, 10:47 a.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Currently Findbugs complains about the a few warnings related to the reliance 
> on the default encoding in the oozie-core module
> They should be fixed to get the code more reliable.
> 
> 
> Diffs
> -
> 
>   core/pom.xml a5a776ca6 
>   core/src/main/java/org/apache/oozie/StringBlob.java 6c776011c 
>   core/src/main/java/org/apache/oozie/action/ActionExecutor.java 919509d35 
>   core/src/main/java/org/apache/oozie/action/hadoop/LauncherHelper.java 
> 21c9b7e4e 
>   
> core/src/main/java/org/apache/oozie/action/hadoop/MapReduceActionExecutor.java
>  d0b807423 
>   core/src/main/java/org/apache/oozie/action/hadoop/SqoopActionExecutor.java 
> 69d5e7e9b 
>   core/src/main/java/org/apache/oozie/action/ssh/SshActionExecutor.java 
> 128feee72 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleSubmitXCommand.java 
> 80e7d5d4b 
>   core/src/main/java/org/apache/oozie/command/coord/CoordSubmitXCommand.java 
> 8bfa634f8 
>   core/src/main/java/org/apache/oozie/command/coord/CoordUpdateXCommand.java 
> 502a800fb 
>   core/src/main/java/org/apache/oozie/service/AuthorizationService.java 
> 251838ce7 
>   core/src/main/java/org/apache/oozie/service/WorkflowAppService.java 
> c725f493e 
>   core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 361141b0d 
>   core/src/main/java/org/apache/oozie/servlet/V2ValidateServlet.java 
> 36a9de22f 
>   core/src/main/java/org/apache/oozie/util/AuthUrlClient.java 4fc8f5726 
>   core/src/main/java/org/apache/oozie/util/IOUtils.java 3674dc48b 
>   core/src/main/java/org/apache/oozie/util/MultiFileReader.java 1ab5a7af2 
>   core/src/main/java/org/apache/oozie/util/XConfiguration.java e3591db32 
>   core/src/main/java/org/apache/oozie/util/XmlUtils.java 9db46b365 
> 
> 
> Diff: https://reviews.apache.org/r/67763/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 67766: OOZIE-2955 Fix Findbugs warnings related to reliance on default encoding in oozie-client

[András Piros via Review Board]

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




client/src/main/java/org/apache/oozie/client/AuthOozieClient.java
Line 218 (original), 221 (patched)


Please address this 
[`FINDBUGS_DIFF`](https://github.com/find-sec-bugs/find-sec-bugs/blob/master/plugin/src/main/resources/metadata/messages.xml#L356-L411)
 issue.


- András Piros


On June 28, 2018, 8:58 a.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67766/
> ---
> 
> (Updated June 28, 2018, 8:58 a.m.)
> 
> 
> Review request for oozie and András Piros.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> Currently Findbugs complains about some warnings related to the reliance on 
> the default encoding in the oozie-client module:
> They should be fixed to get the code more reliable.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/cli/CLIParser.java bbdb8035 
>   client/src/main/java/org/apache/oozie/cli/OozieCLI.java bc234e31 
>   client/src/main/java/org/apache/oozie/client/AuthOozieClient.java 98f421f3 
>   client/src/main/java/org/apache/oozie/client/OozieClient.java 2cc16923 
>   client/src/main/java/org/apache/oozie/client/XOozieClient.java ba60f8e7 
> 
> 
> Diff: https://reviews.apache.org/r/67766/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 66656: Exclusion pattern for sharelib.

[András Piros via Review Board]

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




core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 35 (patched)


Should be `static`.



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 36 (patched)


Could be `private`.



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 54-62 (patched)


Remove comment and extract method `Optional 
tryFindExcludePattern()`.



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 64-67 (patched)


Could use `if (maybeExcludePattern.absent())` instead.



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 76 (patched)


`shouldExclude()` may be a better name.



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 81 (patched)


Does `actionLibURI` always point to a file?



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 85 (patched)


Does `actionLibURI` always point to a file?



core/src/main/java/org/apache/oozie/service/ShareLibService.java
Lines 625 (patched)


A `LOG.debug()` w/ `shareLibRootPath` contents may come on handy here.



core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java
Lines 334-344 (patched)


Please create only one implementation w/ a `Collection` type, and call that 
from the other method.



core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java
Lines 352-362 (patched)


Please implement only one method, and call that from another.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
Line 67 (original), 66 (patched)


Please remove star imports.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 36-50 (patched)


Use `private static final String[] TEST_SHARELIB_JARS` instead.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 52-61 (patched)


Use `private static final String[] TEST_SHARELIB_ARCHIVES` instead.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 67-70 (patched)


Extract method `setHadoopSystemProps()`.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 71-74 (patched)


Extract method `createActionConfDirAndFiles()`.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 78-82 (patched)


Extract method `createSharelibDirs()`.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 84-100 (patched)


Extract method `createSharelibFiles()` and use `foreach` instead.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 110-123 (patched)


Extract method `createContextUsingSharelib()`.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 110-120 (patched)


I feel the need for an `ActionXmlBuilder` class.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 127-132 (patched)


Extract method `createWorkflowJobUsingSharelib()`.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 135-140 (patched)


Extract method `createActionExecutorAndSetupServices()`.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 146 (patched)


Add `message`.