Re: Review Request 68140: OOZIE-3229 Improved filtering options in V2SLAServlet

[Andras Salamon <an...@melda.info>]

> 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 68-164 (original), 156-183 (patched)
> > <https://reviews.apache.org/r/68140/diff/4/?file=2076537#file2076537line170>
> >
> >     I feel the need to extract the creation of the `CriteriaQuery` to another, maybe nested, class - for the sake of better testability.

There are so many references to FilterField enum, that the new class would not be too useful here. I've created smaller methods to make the code a little bit easier to understand.


> 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 342-354 (original), 317-326 (patched)
> > <https://reviews.apache.org/r/68140/diff/4/?file=2076537#file2076537line537>
> >
> >     Part of a nested class, maybe?

I created a nested class FilterCollection to store the hashmap and the get and set filterfield methods. After our discussion I wanted to put the FilterField enum into this class, but I cannot put a static class (enum) into a non-static inner class.


> On Aug. 24, 2018, 2:05 p.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java
> > Lines 222-229 (original), 152-178 (patched)
> > <https://reviews.apache.org/r/68140/diff/4/?file=2076538#file2076538line222>
> >
> >     Part of a nested class, for better testability

I've created a FilterParser class (+unit tests for this class). Later other parts of oozie should also use this class (OOZIE-3335).


> On Aug. 24, 2018, 2:05 p.m., András Piros wrote:
> > webapp/src/main/webapp/console/sla/css/oozie-sla.css
> > Line 45 (original), 45 (patched)
> > <https://reviews.apache.org/r/68140/diff/4/?file=2076542#file2076542line45>
> >
> >     Did you test that on a pseudo-distributed Oozie?

Yes, I tested it when I modified the css/html/js. I'd like to test it one more time before commit.


> On Aug. 24, 2018, 2:05 p.m., András Piros wrote:
> > webapp/src/main/webapp/console/sla/js/oozie-sla.js
> > Lines 29-31 (original), 25-41 (patched)
> > <https://reviews.apache.org/r/68140/diff/4/?file=2076543#file2076543line29>
> >
> >     Did you test that on a pseudo-distributed Oozie?

Yes, I tested it when I modified the css/html/js. I'd like to test it one more time before commit.


- 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
> 
>