> On Aug. 24, 2018, 2:05 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Lines 67-69 (patched) > > <https://reviews.apache.org/r/68140/diff/4/?file=2076537#file2076537line80> > > > > Could be `static`
static is redundant for inner enums > On Aug. 24, 2018, 2:05 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Lines 71 (patched) > > <https://reviews.apache.org/r/68140/diff/4/?file=2076537#file2076537line84> > > > > Could be `static` static is redundant for inner enums > On Aug. 24, 2018, 2:05 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Lines 98 (patched) > > <https://reviews.apache.org/r/68140/diff/4/?file=2076537#file2076537line111> > > > > I'd rather calculate `getColumnName()` as `this.name().toLowerCase()`. eliminated the field, but use the fact that the name of the filter is the lower case version of the enum name. > On Aug. 24, 2018, 2:05 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Lines 99-100 (patched) > > <https://reviews.apache.org/r/68140/diff/4/?file=2076537#file2076537line112> > > > > 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. dbFieldName can only be calculated for the simple filter fields (like app_name). There are lots of exceptions like _START, _END, _MIN, _MAX fields and the very special fields like ID, PARENT_ID, BUNDLE. I've started to implement db type reading, but I reverted it. The code was not too nice, and later I realized that enum fields (SLAStatus, EventStatus) are stored as simple String fields. > On Aug. 24, 2018, 2:05 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Lines 134-139 (patched) > > <https://reviews.apache.org/r/68140/diff/4/?file=2076537#file2076537line147> > > > > Isn't Guava's `Lists#newArrayList(Iterables#filter(original, > > predicate))` a better option? Since we already use the fact (based on your previous suggestion) that the filter name is the lower case version of the enum name, a simple valueOf is enough here (plus a few checks). > On Aug. 24, 2018, 2:05 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Line 202 (original), 190 (patched) > > <https://reviews.apache.org/r/68140/diff/4/?file=2076537#file2076537line307> > > > > We should list the possible `Exception` subclasses rather than one > > single general `catch` clause. Ooops. try-catch is no longer needed here. > On Aug. 24, 2018, 2:05 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java > > Lines 40 (patched) > > <https://reviews.apache.org/r/68140/diff/4/?file=2076539#file2076539line40> > > > > `ignored` We are expecting this exception, so I only added comment here. > On Aug. 24, 2018, 2:05 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java > > Lines 53 (patched) > > <https://reviews.apache.org/r/68140/diff/4/?file=2076539#file2076539line53> > > > > `ignored` We are expecting this exception, so I only added comment here. > On Aug. 24, 2018, 2:05 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java > > Lines 165 (patched) > > <https://reviews.apache.org/r/68140/diff/4/?file=2076539#file2076539line165> > > > > `ignored` We are expecting this exception, so I only added comment here. - Andras ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68140/#review207874 ----------------------------------------------------------- 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 > >
