> On Sept. 4, 2018, 12:25 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Lines 218 (patched) > > <https://reviews.apache.org/r/68140/diff/5/?file=2082094#file2082094line430> > > > > Always `ORDER BY NOMINAL_TIME`?
Added sortby and order parameters similar to job filter. To avoid undefined ordering I always add a second order by nominal_time. I'm not sure about this. > On Sept. 4, 2018, 12:25 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Lines 228-239 (patched) > > <https://reviews.apache.org/r/68140/diff/5/?file=2082094#file2082094line440> > > > > I'm thinking whether `fieldName LIKE '%fieldValue%'` can / should be > > supported. Added support for like. I'm not sure if % and _ is suitable for the users. Maybe we should allow only * (and convert it to %) instead. > On Sept. 4, 2018, 12:25 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Lines 296-354 (patched) > > <https://reviews.apache.org/r/68140/diff/5/?file=2082094#file2082094line512> > > > > Isn't it possible to use the Visitor design pattern instead? Or, at the > > very least, extract the `case` contents to well named methods? Create an enum based strategy like solution. > On Sept. 4, 2018, 12:25 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/servlet/TestV2SLAServletRun.java > > Lines 30 (patched) > > <https://reviews.apache.org/r/68140/diff/5/?file=2082101#file2082101line30> > > > > `TestIntegrationV2SLAServlet` might be a better name Using TestV2SLAServletIntegration instead, to keep it similar to the other test class names. > On Sept. 4, 2018, 12:25 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/servlet/V2SLAServletTestCase.java > > Lines 38 (patched) > > <https://reviews.apache.org/r/68140/diff/5/?file=2082102#file2082102line38> > > > > 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. We need createBundleJob method for instance. I created two classes because half of the methods were using one type of setup method, the other half was using a different type of setup method. > On Sept. 4, 2018, 12:25 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/servlet/V2SLAServletTestCase.java > > Lines 45-46 (patched) > > <https://reviews.apache.org/r/68140/diff/5/?file=2082102#file2082102line45> > > > > Sure it isn't handled by ancestors? Yes, I've tested without this. - Andras ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68140/#review208294 ----------------------------------------------------------- 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 > >
