You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Peter Cseh via Review Board <no...@reviews.apache.org> on 2018/03/20 21:04:33 UTC
Review Request 66181: OOZIE-3056 Implement new mechanism to specify
ShareLibs for workflow actions
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66181/
-----------------------------------------------------------
Review request for oozie, András Piros and Attila Sasvari.
Bugs: OOZIE-3056
https://issues.apache.org/jira/browse/OOZIE-3056
Repository: oozie-git
Description
-------
Refactored out Sharelib name resolution from JavaAE as an additional step to pick that giant class to pieces.
Added logic to process the <sharelib> properties added in <launcher> sections.
Diffs
-----
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 122dfd02e17456b9aab924504faee790813fb6a1
core/src/main/java/org/apache/oozie/action/hadoop/SharelibResolver.java PRE-CREATION
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java b55a3cd7f8064cd898edc686891c9f6ebb118c42
core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java PRE-CREATION
Diff: https://reviews.apache.org/r/66181/diff/1/
Testing
-------
Added Junit tests to validate that
1) the configs are propagating and overwriting each other correctly when an xml is formatted
2) the configs are resolved in the correct order for sharelib names
Thanks,
Peter Cseh
Re: Review Request 66181: OOZIE-3056 Implement new mechanism to
specify ShareLibs for workflow actions
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66181/#review199944
-----------------------------------------------------------
Ship it!
Ship It!
- András Piros
On March 23, 2018, 7:47 a.m., Peter Cseh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66181/
> -----------------------------------------------------------
>
> (Updated March 23, 2018, 7:47 a.m.)
>
>
> Review request for oozie, András Piros and Attila Sasvari.
>
>
> Bugs: OOZIE-3056
> https://issues.apache.org/jira/browse/OOZIE-3056
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Refactored out Sharelib name resolution from JavaAE as an additional step to pick that giant class to pieces.
> Added logic to process the <sharelib> properties added in <launcher> sections.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 122dfd02e17456b9aab924504faee790813fb6a1
> core/src/main/java/org/apache/oozie/action/hadoop/SharelibResolver.java PRE-CREATION
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java b55a3cd7f8064cd898edc686891c9f6ebb118c42
> core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibConfigs.java PRE-CREATION
> core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java PRE-CREATION
> core/src/test/java/org/apache/oozie/action/hadoop/TestWorkflowHelper.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66181/diff/4/
>
>
> Testing
> -------
>
> Added Junit tests to validate that
> 1) the configs are propagating and overwriting each other correctly when an xml is formatted
> 2) the configs are resolved in the correct order for sharelib names
>
>
> Thanks,
>
> Peter Cseh
>
>
Re: Review Request 66181: OOZIE-3056 Implement new mechanism to
specify ShareLibs for workflow actions
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66181/#review199960
-----------------------------------------------------------
Ship it!
Ship It!
- András Piros
On March 23, 2018, 7:47 a.m., Peter Cseh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66181/
> -----------------------------------------------------------
>
> (Updated March 23, 2018, 7:47 a.m.)
>
>
> Review request for oozie, András Piros and Attila Sasvari.
>
>
> Bugs: OOZIE-3056
> https://issues.apache.org/jira/browse/OOZIE-3056
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Refactored out Sharelib name resolution from JavaAE as an additional step to pick that giant class to pieces.
> Added logic to process the <sharelib> properties added in <launcher> sections.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 122dfd02e17456b9aab924504faee790813fb6a1
> core/src/main/java/org/apache/oozie/action/hadoop/SharelibResolver.java PRE-CREATION
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java b55a3cd7f8064cd898edc686891c9f6ebb118c42
> core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibConfigs.java PRE-CREATION
> core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java PRE-CREATION
> core/src/test/java/org/apache/oozie/action/hadoop/TestWorkflowHelper.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66181/diff/4/
>
>
> Testing
> -------
>
> Added Junit tests to validate that
> 1) the configs are propagating and overwriting each other correctly when an xml is formatted
> 2) the configs are resolved in the correct order for sharelib names
>
>
> Thanks,
>
> Peter Cseh
>
>
Re: Review Request 66181: OOZIE-3056 Implement new mechanism to
specify ShareLibs for workflow actions
Posted by Peter Cseh via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66181/
-----------------------------------------------------------
(Updated March 23, 2018, 7:47 a.m.)
Review request for oozie, András Piros and Attila Sasvari.
Bugs: OOZIE-3056
https://issues.apache.org/jira/browse/OOZIE-3056
Repository: oozie-git
Description
-------
Refactored out Sharelib name resolution from JavaAE as an additional step to pick that giant class to pieces.
Added logic to process the <sharelib> properties added in <launcher> sections.
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 122dfd02e17456b9aab924504faee790813fb6a1
core/src/main/java/org/apache/oozie/action/hadoop/SharelibResolver.java PRE-CREATION
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java b55a3cd7f8064cd898edc686891c9f6ebb118c42
core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibConfigs.java PRE-CREATION
core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java PRE-CREATION
core/src/test/java/org/apache/oozie/action/hadoop/TestWorkflowHelper.java PRE-CREATION
Diff: https://reviews.apache.org/r/66181/diff/4/
Changes: https://reviews.apache.org/r/66181/diff/3-4/
Testing
-------
Added Junit tests to validate that
1) the configs are propagating and overwriting each other correctly when an xml is formatted
2) the configs are resolved in the correct order for sharelib names
Thanks,
Peter Cseh
Re: Review Request 66181: OOZIE-3056 Implement new mechanism to
specify ShareLibs for workflow actions
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66181/#review199762
-----------------------------------------------------------
Ship it!
Ship It!
- András Piros
On March 22, 2018, 2:07 p.m., Peter Cseh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66181/
> -----------------------------------------------------------
>
> (Updated March 22, 2018, 2:07 p.m.)
>
>
> Review request for oozie, András Piros and Attila Sasvari.
>
>
> Bugs: OOZIE-3056
> https://issues.apache.org/jira/browse/OOZIE-3056
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Refactored out Sharelib name resolution from JavaAE as an additional step to pick that giant class to pieces.
> Added logic to process the <sharelib> properties added in <launcher> sections.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 122dfd02e17456b9aab924504faee790813fb6a1
> core/src/main/java/org/apache/oozie/action/hadoop/SharelibResolver.java PRE-CREATION
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java b55a3cd7f8064cd898edc686891c9f6ebb118c42
> core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibConfigs.java PRE-CREATION
> core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java PRE-CREATION
> core/src/test/java/org/apache/oozie/action/hadoop/TestWorkflowHelper.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66181/diff/3/
>
>
> Testing
> -------
>
> Added Junit tests to validate that
> 1) the configs are propagating and overwriting each other correctly when an xml is formatted
> 2) the configs are resolved in the correct order for sharelib names
>
>
> Thanks,
>
> Peter Cseh
>
>
Re: Review Request 66181: OOZIE-3056 Implement new mechanism to
specify ShareLibs for workflow actions
Posted by Peter Cseh via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66181/
-----------------------------------------------------------
(Updated March 22, 2018, 2:07 p.m.)
Review request for oozie, András Piros and Attila Sasvari.
Bugs: OOZIE-3056
https://issues.apache.org/jira/browse/OOZIE-3056
Repository: oozie-git
Description
-------
Refactored out Sharelib name resolution from JavaAE as an additional step to pick that giant class to pieces.
Added logic to process the <sharelib> properties added in <launcher> sections.
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 122dfd02e17456b9aab924504faee790813fb6a1
core/src/main/java/org/apache/oozie/action/hadoop/SharelibResolver.java PRE-CREATION
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java b55a3cd7f8064cd898edc686891c9f6ebb118c42
core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibConfigs.java PRE-CREATION
core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java PRE-CREATION
core/src/test/java/org/apache/oozie/action/hadoop/TestWorkflowHelper.java PRE-CREATION
docs/src/site/twiki/AG_Install.twiki 8e2d795da838d184940bc177e95e9dfb8b52cc54
docs/src/site/twiki/DG_DistCpActionExtension.twiki 8bab3da448f5491164406860309cfce76b85e464
docs/src/site/twiki/DG_Examples.twiki 13dfa28c0eecab70321886b2806a791346c46d3b
docs/src/site/twiki/DG_Hive2ActionExtension.twiki efbe56dabdd7e54f56d2351694ed3b5d722c902b
docs/src/site/twiki/DG_HiveActionExtension.twiki aaa74fa7b7046394c265128c4517ba667508b0f0
docs/src/site/twiki/DG_ShellActionExtension.twiki 5894c280a52b28e7507b2b5f078a0cc6d65cf93c
docs/src/site/twiki/DG_SparkActionExtension.twiki 66076ff9d3098aeb06301458b900da8d7f5fb5db
docs/src/site/twiki/DG_SqoopActionExtension.twiki 0317d0ceccf065a954976dcf6ef1cf995a169d14
docs/src/site/twiki/DG_SshActionExtension.twiki 99b96f7fa398de9a1771359894f46352e9dbd9cd
docs/src/site/twiki/ENG_MiniOozie.twiki 0b1628986bf31a4a0874be7539fae0af57e51bd0
docs/src/site/twiki/WorkflowFunctionalSpec.twiki 5df6024bf22c56f1fffee6487d18157c8485d520
release-log.txt 76401f5eb92cdea16bc8e33aa92f3d8e5702a28a
Diff: https://reviews.apache.org/r/66181/diff/2/
Changes: https://reviews.apache.org/r/66181/diff/1-2/
Testing
-------
Added Junit tests to validate that
1) the configs are propagating and overwriting each other correctly when an xml is formatted
2) the configs are resolved in the correct order for sharelib names
Thanks,
Peter Cseh
Re: Review Request 66181: OOZIE-3056 Implement new mechanism to
specify ShareLibs for workflow actions
Posted by Peter Cseh via Review Board <no...@reviews.apache.org>.
> On March 21, 2018, 1:28 p.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
> > Lines 1862-1875 (original), 1846-1848 (patched)
> > <https://reviews.apache.org/r/66181/diff/1/?file=1983709#file1983709line1863>
> >
> > Remove `catch` block as it's already handled by `SharelibResolver#resolve()`.
It turns out that I don't need the try-catch in the Resolver.
> On March 21, 2018, 1:28 p.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/SharelibResolver.java
> > Lines 76-78 (patched)
> > <https://reviews.apache.org/r/66181/diff/1/?file=1983710#file1983710line76>
> >
> > What about `[""]`, that is, a `String[]` consisting of only empty `String` instances?
conf.getStrings() does not give back empty strings in the arrays.
> On March 21, 2018, 1:28 p.m., András Piros wrote:
> > core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
> > Lines 1251-1265 (patched)
> > <https://reviews.apache.org/r/66181/diff/1/?file=1983711#file1983711line1251>
> >
> > What about a new test class that `extends MiniOozieTestCase` instead? `TestJavaActionExecutor` is lengthy enough already.
Done.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66181/#review199656
-----------------------------------------------------------
On March 20, 2018, 9:04 p.m., Peter Cseh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66181/
> -----------------------------------------------------------
>
> (Updated March 20, 2018, 9:04 p.m.)
>
>
> Review request for oozie, András Piros and Attila Sasvari.
>
>
> Bugs: OOZIE-3056
> https://issues.apache.org/jira/browse/OOZIE-3056
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Refactored out Sharelib name resolution from JavaAE as an additional step to pick that giant class to pieces.
> Added logic to process the <sharelib> properties added in <launcher> sections.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 122dfd02e17456b9aab924504faee790813fb6a1
> core/src/main/java/org/apache/oozie/action/hadoop/SharelibResolver.java PRE-CREATION
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java b55a3cd7f8064cd898edc686891c9f6ebb118c42
> core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66181/diff/1/
>
>
> Testing
> -------
>
> Added Junit tests to validate that
> 1) the configs are propagating and overwriting each other correctly when an xml is formatted
> 2) the configs are resolved in the correct order for sharelib names
>
>
> Thanks,
>
> Peter Cseh
>
>
Re: Review Request 66181: OOZIE-3056 Implement new mechanism to
specify ShareLibs for workflow actions
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66181/#review199656
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1860-1861 (original), 1844-1845 (patched)
<https://reviews.apache.org/r/66181/#comment280015>
Extract parameters to local variables w/ descriptive names to enhance readability.
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1862-1875 (original), 1846-1848 (patched)
<https://reviews.apache.org/r/66181/#comment280019>
Remove `catch` block as it's already handled by `SharelibResolver#resolve()`.
core/src/main/java/org/apache/oozie/action/hadoop/SharelibResolver.java
Lines 71 (patched)
<https://reviews.apache.org/r/66181/#comment280018>
:D replace `It cannot happen` with something less fun-prone in customer logs.
core/src/main/java/org/apache/oozie/action/hadoop/SharelibResolver.java
Lines 76-78 (patched)
<https://reviews.apache.org/r/66181/#comment280020>
What about `[""]`, that is, a `String[]` consisting of only empty `String` instances?
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
Lines 1234 (patched)
<https://reviews.apache.org/r/66181/#comment280035>
Typo: `globalConfigForJavaSharelib`
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
Lines 1251-1265 (patched)
<https://reviews.apache.org/r/66181/#comment280036>
What about a new test class that `extends MiniOozieTestCase` instead? `TestJavaActionExecutor` is lengthy enough already.
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
Lines 1253-1262 (patched)
<https://reviews.apache.org/r/66181/#comment280037>
Should test for the case there are no sharelib settings defined anywhere.
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
Lines 1283-1289 (patched)
<https://reviews.apache.org/r/66181/#comment280031>
You can `waitFor(20_000, ...)`.
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
Lines 1291 (patched)
<https://reviews.apache.org/r/66181/#comment280032>
Give a `reason` and use `assertThat(String reason, T actual, Matcher<? super T> matcher)`.
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
Lines 1292 (patched)
<https://reviews.apache.org/r/66181/#comment280033>
Give a `reason` and use `assertThat(String reason, T actual, Matcher<? super T> matcher)`.
core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java
Line 2250 (original), 2323-2325 (patched)
<https://reviews.apache.org/r/66181/#comment280029>
Extracting to local variable helps, e.g. `String javaActionConfig`.
core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java
Lines 31 (patched)
<https://reviews.apache.org/r/66181/#comment280027>
Give a `reason` and use `assertThat(String reason, T actual, Matcher<? super T> matcher)`.
core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java
Lines 34 (patched)
<https://reviews.apache.org/r/66181/#comment280022>
Give a `reason` and use `assertThat(String reason, T actual, Matcher<? super T> matcher)`.
core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java
Lines 37 (patched)
<https://reviews.apache.org/r/66181/#comment280023>
Give a `reason` and use `assertThat(String reason, T actual, Matcher<? super T> matcher)`.
core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java
Lines 40 (patched)
<https://reviews.apache.org/r/66181/#comment280024>
Give a `reason` and use `assertThat(String reason, T actual, Matcher<? super T> matcher)`.
core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java
Lines 43 (patched)
<https://reviews.apache.org/r/66181/#comment280025>
Give a `reason` and use `assertThat(String reason, T actual, Matcher<? super T> matcher)`.
core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java
Lines 46 (patched)
<https://reviews.apache.org/r/66181/#comment280026>
Give a `reason` and use `assertThat(String reason, T actual, Matcher<? super T> matcher)`.
- András Piros
On March 20, 2018, 9:04 p.m., Peter Cseh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66181/
> -----------------------------------------------------------
>
> (Updated March 20, 2018, 9:04 p.m.)
>
>
> Review request for oozie, András Piros and Attila Sasvari.
>
>
> Bugs: OOZIE-3056
> https://issues.apache.org/jira/browse/OOZIE-3056
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Refactored out Sharelib name resolution from JavaAE as an additional step to pick that giant class to pieces.
> Added logic to process the <sharelib> properties added in <launcher> sections.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 122dfd02e17456b9aab924504faee790813fb6a1
> core/src/main/java/org/apache/oozie/action/hadoop/SharelibResolver.java PRE-CREATION
> core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java b55a3cd7f8064cd898edc686891c9f6ebb118c42
> core/src/test/java/org/apache/oozie/action/hadoop/TestSharelibResolver.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66181/diff/1/
>
>
> Testing
> -------
>
> Added Junit tests to validate that
> 1) the configs are propagating and overwriting each other correctly when an xml is formatted
> 2) the configs are resolved in the correct order for sharelib names
>
>
> Thanks,
>
> Peter Cseh
>
>