----------------------------------------------------------- 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) <https://reviews.apache.org/r/68140/#comment292030> Use `PARAMETER_EQUALS` core/src/main/java/org/apache/oozie/FilterParser.java Lines 56 (patched) <https://reviews.apache.org/r/68140/#comment292031> `Strings.isNullOrEmpty()` core/src/main/java/org/apache/oozie/FilterParser.java Lines 74-78 (patched) <https://reviews.apache.org/r/68140/#comment292032> 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) <https://reviews.apache.org/r/68140/#comment292033> 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) <https://reviews.apache.org/r/68140/#comment292049> Could be `final` core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 122 (patched) <https://reviews.apache.org/r/68140/#comment292050> Could be `final` core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 128 (patched) <https://reviews.apache.org/r/68140/#comment292051> Can be `final` core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 130 (patched) <https://reviews.apache.org/r/68140/#comment292052> `field.type.equals()` core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 133 (patched) <https://reviews.apache.org/r/68140/#comment292053> `field.type.equals()` core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 137 (patched) <https://reviews.apache.org/r/68140/#comment292054> `field.type.equals()` core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 213-214 (patched) <https://reviews.apache.org/r/68140/#comment292055> Would extract to some method like `ensureCriteriaFields()` core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 215-216 (patched) <https://reviews.apache.org/r/68140/#comment292056> Extract to `createSelectFrom()` core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 218 (patched) <https://reviews.apache.org/r/68140/#comment292057> Always `ORDER BY NOMINAL_TIME`? core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 228-239 (patched) <https://reviews.apache.org/r/68140/#comment292058> 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) <https://reviews.apache.org/r/68140/#comment292059> `FilterField.PARENT_ID.dbFieldName` core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 273 (patched) <https://reviews.apache.org/r/68140/#comment292060> 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) <https://reviews.apache.org/r/68140/#comment292061> Special handling of unparseable `event_status` value core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 296-354 (patched) <https://reviews.apache.org/r/68140/#comment292062> 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) <https://reviews.apache.org/r/68140/#comment292063> Needn't and shouldn't `extends XTestCase` core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 45 (patched) <https://reviews.apache.org/r/68140/#comment292064> Rather use `ExpectedException` core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 47-50 (patched) <https://reviews.apache.org/r/68140/#comment292068> Instead of this comment, rather use a name like `ignored` core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 57 (patched) <https://reviews.apache.org/r/68140/#comment292065> Rather use `ExpectedException` core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 59-62 (patched) <https://reviews.apache.org/r/68140/#comment292069> Instead of this comment, rather use a name like `ignored` core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 69 (patched) <https://reviews.apache.org/r/68140/#comment292066> Rather use `ExpectedException` core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 71-74 (patched) <https://reviews.apache.org/r/68140/#comment292070> Instead of this comment, rather use a name like `ignored` core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 81 (patched) <https://reviews.apache.org/r/68140/#comment292067> Rather use `ExpectedException` core/src/test/java/org/apache/oozie/TestFilterParser.java Lines 83-86 (patched) <https://reviews.apache.org/r/68140/#comment292071> Instead of this comment, rather use a name like `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 31 (patched) <https://reviews.apache.org/r/68140/#comment292096> I'm wondering whether `TestSLASummaryGetForFilterJPAExecutorFilterCollection` already covers that part of the code in sufficient detail core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 44-47 (patched) <https://reviews.apache.org/r/68140/#comment292072> Instead of this comment, rather use a name like `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 57-60 (patched) <https://reviews.apache.org/r/68140/#comment292073> Instead of this comment, rather use a name like `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 70-73 (patched) <https://reviews.apache.org/r/68140/#comment292074> Instead of this comment, rather use a name like `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 84-87 (patched) <https://reviews.apache.org/r/68140/#comment292075> Instead of this comment, rather use a name like `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 141-144 (patched) <https://reviews.apache.org/r/68140/#comment292078> Instead of this comment, rather use a name like `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 152-161 (patched) <https://reviews.apache.org/r/68140/#comment292079> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 165-175 (patched) <https://reviews.apache.org/r/68140/#comment292080> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 179-189 (patched) <https://reviews.apache.org/r/68140/#comment292081> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 193-203 (patched) <https://reviews.apache.org/r/68140/#comment292082> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 207-218 (patched) <https://reviews.apache.org/r/68140/#comment292083> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 222-233 (patched) <https://reviews.apache.org/r/68140/#comment292084> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 31 (patched) <https://reviews.apache.org/r/68140/#comment292085> Use single class imports core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 33 (patched) <https://reviews.apache.org/r/68140/#comment292088> Do we need `extends XTestCase`? I'm wondering whether `TestSLASummaryGetForFilterJPAExecutor` already covers that part of the code in sufficient detail core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 46 (patched) <https://reviews.apache.org/r/68140/#comment292089> `ExpectedException` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 47-50 (patched) <https://reviews.apache.org/r/68140/#comment292086> `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 59 (patched) <https://reviews.apache.org/r/68140/#comment292090> `ExpectedException` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 60-63 (patched) <https://reviews.apache.org/r/68140/#comment292087> `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 72 (patched) <https://reviews.apache.org/r/68140/#comment292091> `ExpectedException` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 73-76 (patched) <https://reviews.apache.org/r/68140/#comment292094> `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 85 (patched) <https://reviews.apache.org/r/68140/#comment292092> `ExpectedException` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 87-90 (patched) <https://reviews.apache.org/r/68140/#comment292095> `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 142 (patched) <https://reviews.apache.org/r/68140/#comment292093> `ExpectedException` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 144-147 (patched) <https://reviews.apache.org/r/68140/#comment292097> `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 155-164 (patched) <https://reviews.apache.org/r/68140/#comment292098> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 168-178 (patched) <https://reviews.apache.org/r/68140/#comment292099> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 182-192 (patched) <https://reviews.apache.org/r/68140/#comment292100> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 196-205 (patched) <https://reviews.apache.org/r/68140/#comment292101> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 209-219 (patched) <https://reviews.apache.org/r/68140/#comment292102> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutorFilterCollection.java Lines 223-233 (patched) <https://reviews.apache.org/r/68140/#comment292103> Could extract method `checkSetAndAssertFilterField(String, List<String>)` core/src/test/java/org/apache/oozie/servlet/TestV2SLAServlet.java Lines 34 (patched) <https://reviews.apache.org/r/68140/#comment292104> `TestV2SLAServletSLAJSONResponse` would be a better name core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 128 (patched) <https://reviews.apache.org/r/68140/#comment292105> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 140-142 (patched) <https://reviews.apache.org/r/68140/#comment292106> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 184-186 (patched) <https://reviews.apache.org/r/68140/#comment292107> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 196 (patched) <https://reviews.apache.org/r/68140/#comment292108> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 199 (patched) <https://reviews.apache.org/r/68140/#comment292109> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 201 (patched) <https://reviews.apache.org/r/68140/#comment292110> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 203 (patched) <https://reviews.apache.org/r/68140/#comment292111> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 211 (patched) <https://reviews.apache.org/r/68140/#comment292112> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 214 (patched) <https://reviews.apache.org/r/68140/#comment292113> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 216 (patched) <https://reviews.apache.org/r/68140/#comment292114> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 218 (patched) <https://reviews.apache.org/r/68140/#comment292115> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 226 (patched) <https://reviews.apache.org/r/68140/#comment292116> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 229 (patched) <https://reviews.apache.org/r/68140/#comment292117> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 231-234 (patched) <https://reviews.apache.org/r/68140/#comment292118> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 242 (patched) <https://reviews.apache.org/r/68140/#comment292119> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 246 (patched) <https://reviews.apache.org/r/68140/#comment292120> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 249 (patched) <https://reviews.apache.org/r/68140/#comment292121> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 259 (patched) <https://reviews.apache.org/r/68140/#comment292122> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 261-262 (patched) <https://reviews.apache.org/r/68140/#comment292123> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java Lines 264-269 (patched) <https://reviews.apache.org/r/68140/#comment292124> Assertion message missing core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletRun.java Lines 30 (patched) <https://reviews.apache.org/r/68140/#comment292125> `TestIntegrationV2SLAServlet` might be a better name core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletRun.java Lines 35-44 (patched) <https://reviews.apache.org/r/68140/#comment292126> Extract method `callHttpEndpointAndAssertResponse(Map<String, String> filterParams, int expectedResponseCode)` core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletRun.java Lines 48-58 (patched) <https://reviews.apache.org/r/68140/#comment292127> Extract method `callHttpEndpointAndAssertResponse(Map<String, String> filterParams, int expectedResponseCode)` core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletRun.java Lines 62-72 (patched) <https://reviews.apache.org/r/68140/#comment292128> Extract method `callHttpEndpointAndAssertResponse(Map<String, String> filterParams, int expectedResponseCode)` core/src/test/java/org/apache/oozie/servlet/V2SLAServletTestCase.java Lines 38 (patched) <https://reviews.apache.org/r/68140/#comment292132> I'm wondering why we have to `extends XDataTestCase` and why this class has to be extended instead of being a composite in the caller class. core/src/test/java/org/apache/oozie/servlet/V2SLAServletTestCase.java Lines 45-46 (patched) <https://reviews.apache.org/r/68140/#comment292133> Sure it isn't handled by ancestors? core/src/test/java/org/apache/oozie/servlet/V2SLAServletTestCase.java Lines 112 (patched) <https://reviews.apache.org/r/68140/#comment292130> Assertion message missing core/src/test/java/org/apache/oozie/servlet/V2SLAServletTestCase.java Lines 121-133 (patched) <https://reviews.apache.org/r/68140/#comment292131> Assertion message missing docs/src/site/twiki/DG_SLAMonitoring.twiki Lines 175 (patched) <https://reviews.apache.org/r/68140/#comment292134> Login name? First / last name? docs/src/site/twiki/DG_SLAMonitoring.twiki Lines 176-182 (patched) <https://reviews.apache.org/r/68140/#comment292135> Any checks between start and end timestamps? docs/src/site/twiki/DG_SLAMonitoring.twiki Lines 184 (patched) <https://reviews.apache.org/r/68140/#comment292136> `=event_status=`, `=sla_status=` docs/src/site/twiki/DG_SLAMonitoring.twiki Lines 345 (patched) <https://reviews.apache.org/r/68140/#comment292137> The latter `actualstart_start` has to be `actualstart_end` - András Piros On Sept. 4, 2018, 8:36 a.m., Andras Salamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68140/ > ----------------------------------------------------------- > > (Updated Sept. 4, 2018, 8:36 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 949b4532 > core/src/main/java/org/apache/oozie/FilterParser.java PRE-CREATION > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > b54161e9 > core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java 3982d1e0 > core/src/test/java/org/apache/oozie/TestFilterParser.java PRE-CREATION > > core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.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 aa633225 > core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletBundle.java > PRE-CREATION > core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletRun.java > PRE-CREATION > core/src/test/java/org/apache/oozie/servlet/V2SLAServletTestCase.java > PRE-CREATION > docs/src/site/twiki/DG_SLAMonitoring.twiki c91c227d > webapp/src/main/webapp/console/sla/css/oozie-sla.css d2f2deee > webapp/src/main/webapp/console/sla/js/oozie-sla.js 2ecad228 > webapp/src/main/webapp/console/sla/oozie-sla.html e5bf6275 > > > Diff: https://reviews.apache.org/r/68140/diff/5/ > > > Testing > ------- > > > Thanks, > > Andras Salamon > >
