----------------------------------------------------------- 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) <https://reviews.apache.org/r/68140/#comment292832> 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) <https://reviews.apache.org/r/68140/#comment292833> 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) <https://reviews.apache.org/r/68140/#comment292834> 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) <https://reviews.apache.org/r/68140/#comment292840> Should be `static class` if possible. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Line 116 (original), 203 (patched) <https://reviews.apache.org/r/68140/#comment292841> `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) <https://reviews.apache.org/r/68140/#comment292835> Why not use `SLASummaryGetForFilterJPAExecutor.class.getSimpleName()` instead? core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 32 (patched) <https://reviews.apache.org/r/68140/#comment292836> Why not use `@Test` annotations and not `extends TestCase` instead? core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 83-86 (patched) <https://reviews.apache.org/r/68140/#comment292837> This stuff would be also simpler using Guava's `OrderedSetMultimap`. core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 91-96 (patched) <https://reviews.apache.org/r/68140/#comment292838> This stuff would be also simpler using Guava's `OrderedSetMultimap`. core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 101-104 (patched) <https://reviews.apache.org/r/68140/#comment292839> 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) <https://reviews.apache.org/r/68140/#comment292842> Is it still needed as we use `ExpectedException`? core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 69-71 (patched) <https://reviews.apache.org/r/68140/#comment292843> Is it still needed as we use `ExpectedException`? core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 80-82 (patched) <https://reviews.apache.org/r/68140/#comment292844> Is it still needed as we use `ExpectedException`? core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 90-92 (patched) <https://reviews.apache.org/r/68140/#comment292845> Is it still needed as we use `ExpectedException`? core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 136-138 (patched) <https://reviews.apache.org/r/68140/#comment292846> Is it still needed as we use `ExpectedException`? core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 147-149 (patched) <https://reviews.apache.org/r/68140/#comment292847> Is it still needed as we use `ExpectedException`? core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 193-195 (patched) <https://reviews.apache.org/r/68140/#comment292848> Is it still needed as we use `ExpectedException`? core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 207-209 (patched) <https://reviews.apache.org/r/68140/#comment292849> Is it still needed as we use `ExpectedException`? core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 221-223 (patched) <https://reviews.apache.org/r/68140/#comment292850> Is it still needed as we use `ExpectedException`? core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 232-234 (patched) <https://reviews.apache.org/r/68140/#comment292851> Is it still needed as we use `ExpectedException`? webapp/src/main/webapp/console/sla/js/oozie-sla.js Lines 36 (patched) <https://reviews.apache.org/r/68140/#comment292852> Please insert spaces around Javascript string concatenation. webapp/src/main/webapp/console/sla/js/oozie-sla.js Lines 38 (patched) <https://reviews.apache.org/r/68140/#comment292853> Please insert spaces around Javascript string concatenation. webapp/src/main/webapp/console/sla/js/oozie-sla.js Line 54 (original), 54 (patched) <https://reviews.apache.org/r/68140/#comment292854> Please insert spaces around Javascript string concatenation. - András Piros On Sept. 18, 2018, 7:46 a.m., Andras Salamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68140/ > ----------------------------------------------------------- > > (Updated Sept. 18, 2018, 7:46 a.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/6/ > > > Testing > ------- > > > Thanks, > > Andras Salamon > >
