----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68140/#review207886 -----------------------------------------------------------
I'm not yet finished, but I save a few comments. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 67-69 (patched) <https://reviews.apache.org/r/68140/#comment291501> static is redundant for inner enums core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 71 (patched) <https://reviews.apache.org/r/68140/#comment291502> static is redundant for inner enums core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 98 (patched) <https://reviews.apache.org/r/68140/#comment291655> I eliminated the field, but use the fact that the name of the filter is the lower case version of the enum name. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 99-100 (patched) <https://reviews.apache.org/r/68140/#comment291689> 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. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 134-139 (patched) <https://reviews.apache.org/r/68140/#comment291654> 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). core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Line 202 (original), 190 (patched) <https://reviews.apache.org/r/68140/#comment291498> Ooops. try-catch is no longer needed here. core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 40 (patched) <https://reviews.apache.org/r/68140/#comment291653> We are expecting this exception, so I only added comment here. core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 53 (patched) <https://reviews.apache.org/r/68140/#comment291652> We are expecting this exception, so I only added a comment here. core/src/test/java/org/apache/oozie/executor/jpa/sla/TestSLASummaryGetForFilterJPAExecutor.java Lines 165 (patched) <https://reviews.apache.org/r/68140/#comment291656> We are expecting this exception, so I only added a comment here. - Andras Salamon 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 > >
