----------------------------------------------------------- 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) <https://reviews.apache.org/r/69594/#comment296709> 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) <https://reviews.apache.org/r/69594/#comment296704> What's the semantics of `T` and `U`? core/src/main/java/org/apache/oozie/command/PurgeXCommand.java Lines 82-84 (patched) <https://reviews.apache.org/r/69594/#comment296705> Can be `final` core/src/main/java/org/apache/oozie/command/PurgeXCommand.java Lines 86 (patched) <https://reviews.apache.org/r/69594/#comment296706> Can all be `final` core/src/main/java/org/apache/oozie/command/PurgeXCommand.java Lines 93-94 (patched) <https://reviews.apache.org/r/69594/#comment296703> 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) <https://reviews.apache.org/r/69594/#comment296707> 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) <https://reviews.apache.org/r/69594/#comment296708> 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) <https://reviews.apache.org/r/69594/#comment296710> 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) <https://reviews.apache.org/r/69594/#comment296713> Please extract well-named variables. core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java Lines 2908 (patched) <https://reviews.apache.org/r/69594/#comment296714> Please extract well-named variables. core/src/test/java/org/apache/oozie/command/TestPurgeXCommand.java Lines 2939 (patched) <https://reviews.apache.org/r/69594/#comment296715> Please extract well-named variables. core/src/test/java/org/apache/oozie/command/TestSelectorTreeTraverser.java Lines 37-61 (patched) <https://reviews.apache.org/r/69594/#comment296716> 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 > >
