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