----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68140/#review207874 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 67-69 (patched) <https://reviews.apache.org/r/68140/#comment291431> Could be `static` core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 71 (patched) <https://reviews.apache.org/r/68140/#comment291432> Could be `static` core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 98 (patched) <https://reviews.apache.org/r/68140/#comment291433> I'd rather calculate `getColumnName()` as `this.name().toLowerCase()`. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 99-100 (patched) <https://reviews.apache.org/r/68140/#comment291434> Aren't `type` and `dbFieldName` somehow computable / readable via reflection from the actual `Entity` classes? This would provide a single reason for change as in SRP, single point being only the `Entity` classes' field definitions. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 112-130 (patched) <https://reviews.apache.org/r/68140/#comment291437> Shouldn't we use the Criteria API to return the appropriate type (after conversion if needed) instead? core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 134-139 (patched) <https://reviews.apache.org/r/68140/#comment291436> Isn't Guava's `Lists#newArrayList(Iterables#filter(original, predicate))` a better option? core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 68-164 (original), 156-183 (patched) <https://reviews.apache.org/r/68140/#comment291439> I feel the need to extract the creation of the `CriteriaQuery` to another, maybe nested, class - for the sake of better testability. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Line 200 (original), 188 (patched) <https://reviews.apache.org/r/68140/#comment291440> You can use placeholder like `{0}` instead of `String` concatenation. Otherwise, big thumbs up :) core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Line 202 (original), 190 (patched) <https://reviews.apache.org/r/68140/#comment291441> We should list the possible `Exception` subclasses rather than one single general `catch` clause. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 214-217 (original), 196-200 (patched) <https://reviews.apache.org/r/68140/#comment291442> Needs better, more intuitive naming core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 202 (patched) <https://reviews.apache.org/r/68140/#comment291443> Needs better, more intuitive naming core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Line 245 (original), 220 (patched) <https://reviews.apache.org/r/68140/#comment291444> Needs better, more intuitive naming core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Line 330 (original), 238 (patched) <https://reviews.apache.org/r/68140/#comment291445> Needs better, more intuitive naming core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 246-249 (patched) <https://reviews.apache.org/r/68140/#comment291446> Extract magic `String`s to constant values core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 253-257 (patched) <https://reviews.apache.org/r/68140/#comment291447> Extract magic `String`s to constant values core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 259-261 (patched) <https://reviews.apache.org/r/68140/#comment291448> Extract magic `String`s to constant values core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 266-268 (patched) <https://reviews.apache.org/r/68140/#comment291449> Extract magic `String`s to constant values core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 273-275 (patched) <https://reviews.apache.org/r/68140/#comment291450> Extract magic `String`s to constant values core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 278 (patched) <https://reviews.apache.org/r/68140/#comment291451> Extract magic `String`s to constant values core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 283-285 (patched) <https://reviews.apache.org/r/68140/#comment291452> Extract magic `String`s to constant values core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 290-292 (patched) <https://reviews.apache.org/r/68140/#comment291453> Extract magic `String`s to constant values core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 295-297 (patched) <https://reviews.apache.org/r/68140/#comment291454> Extract magic `String`s to constant values core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 302 (patched) <https://reviews.apache.org/r/68140/#comment291455> Extract to a well-named method so that it's understandable core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Line 332 (original), 308-311 (patched) <https://reviews.apache.org/r/68140/#comment291456> We should also `LOG.error()` and look that we have at least the filter `name` in the log message. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 342-354 (original), 317-326 (patched) <https://reviews.apache.org/r/68140/#comment291457> Part of a nested class, maybe? core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java Line 140 (original), 111 (patched) <https://reviews.apache.org/r/68140/#comment291458> Need to extract `value.get(0)` to a well-named variable core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java Lines 222-229 (original), 152-178 (patched) <https://reviews.apache.org/r/68140/#comment291459> Part of a nested class, for better testability core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java Line 223 (original), 153 (patched) <https://reviews.apache.org/r/68140/#comment291463> Well-named variable core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java Lines 161-162 (patched) <https://reviews.apache.org/r/68140/#comment291461> Extract method core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java Lines 164 (patched) <https://reviews.apache.org/r/68140/#comment291465> Well-named variable core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java Lines 172-173 (patched) <https://reviews.apache.org/r/68140/#comment291462> Reuse extracted method core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 31 (patched) <https://reviews.apache.org/r/68140/#comment291483> Nice job in general. I'd rather separately test the extracted `CriteriaQuery` construction related nested class in a test class which doesn't necessarily needs to `extend XDataTestCase`, and here only test the integration part, calling `entityManager` w/ mock `CriteriaQuery` instances. core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 34-35 (patched) <https://reviews.apache.org/r/68140/#comment291471> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 40 (patched) <https://reviews.apache.org/r/68140/#comment291470> `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 47-48 (patched) <https://reviews.apache.org/r/68140/#comment291472> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 53 (patched) <https://reviews.apache.org/r/68140/#comment291485> `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 60-61 (patched) <https://reviews.apache.org/r/68140/#comment291473> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 63-83 (patched) <https://reviews.apache.org/r/68140/#comment291487> Those `setFilterField()` / `assertEquals()` pairs could well be extracted to a method core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 91-92 (patched) <https://reviews.apache.org/r/68140/#comment291474> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 94-105 (patched) <https://reviews.apache.org/r/68140/#comment291488> Those `setFilterField()` / `assertEquals()` pairs could well be extracted to a method core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 113-114 (patched) <https://reviews.apache.org/r/68140/#comment291475> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 120-151 (patched) <https://reviews.apache.org/r/68140/#comment291489> Those `setFilterField()` / `assertEquals()` pairs could well be extracted to a method core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 159-160 (patched) <https://reviews.apache.org/r/68140/#comment291476> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 165 (patched) <https://reviews.apache.org/r/68140/#comment291486> `ignored` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 173-174 (patched) <https://reviews.apache.org/r/68140/#comment291477> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 188-189 (patched) <https://reviews.apache.org/r/68140/#comment291478> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 204-205 (patched) <https://reviews.apache.org/r/68140/#comment291479> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 220-221 (patched) <https://reviews.apache.org/r/68140/#comment291480> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 236-237 (patched) <https://reviews.apache.org/r/68140/#comment291481> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 253-254 (patched) <https://reviews.apache.org/r/68140/#comment291482> Should rather be a field initialized in `setUp()` core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 265-266 (patched) <https://reviews.apache.org/r/68140/#comment291490> Formatting errors docs/src/site/twiki/DG_SLAMonitoring.twiki Lines 173-186 (patched) <https://reviews.apache.org/r/68140/#comment291491> Would need even more examples here covering single aspects and a mix of those. docs/src/site/twiki/DG_SLAMonitoring.twiki Lines 450 (patched) <https://reviews.apache.org/r/68140/#comment291492> `,` to be placed to the end of the previous line webapp/src/main/webapp/console/sla/css/oozie-sla.css Line 45 (original), 45 (patched) <https://reviews.apache.org/r/68140/#comment291494> Did you test that on a pseudo-distributed Oozie? webapp/src/main/webapp/console/sla/js/oozie-sla.js Lines 29-31 (original), 25-41 (patched) <https://reviews.apache.org/r/68140/#comment291493> Did you test that on a pseudo-distributed Oozie? - András Piros On Aug. 23, 2018, 3:27 p.m., Andras Salamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68140/ > ----------------------------------------------------------- > > (Updated Aug. 23, 2018, 3:27 p.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/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > b54161e9 > core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java 3982d1e0 > > core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java > PRE-CREATION > core/src/test/java/org/apache/oozie/servlet/TestV2SLAServlet.java aa633225 > 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/4/ > > > Testing > ------- > > > Thanks, > > Andras Salamon > >
