> On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > client/src/main/java/org/apache/oozie/client/OozieClient.java > > Lines 177-204 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068364#file2068364line177> > > > > Can you please structure that a bit more? Like having not `String` but > > `enum` instances, having a nested wrapper class...
Removed the filter field listing from this class. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/conf/action-conf/hive.xml > > Line 28 (original), 28 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068365#file2068365line28> > > > > Please revert this change. Ooops. Missing git merge. Reverted. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/action/ActionExecutor.java > > Line 606 (original), 606 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068366#file2068366line606> > > > > Please revert this change. Ooops. Missing git merge. Reverted. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > > Line 1780 (original), 1780 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068367#file2068367line1780> > > > > Please revert this change. Ooops. Missing git merge. Reverted. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > > Lines 1831-1834 (original) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068367#file2068367line1831> > > > > Please revert this change. Ooops. Missing git merge. Reverted. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Lines 51-78 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068368#file2068368line52> > > > > Can we extract that to a state object? Too many fields here, we're far > > away from Single Responsibility Principle. Instead of using lots of fields, I store the (converted) values in a HashMap, which means we don't have to add a new field if we add additional filter fields. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Lines 267-277 (original), 294-311 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068368#file2068368line340> > > > > Please extract method. jobExists variable has been eliminated. Most of this code moved to the SQL builder class. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > > Lines 425-426 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068368#file2068368line475> > > > > Here we're changing existing behavior. Any chance to create separate > > test for the change, plus document that? I think the existing behaviour was incorrect, the second set call should overwrite the value and not only modify it. I've created a test method to check this. Same applies for the eventStatus field. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java > > Line 115 (original), 131 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068369#file2068369line132> > > > > Can we please apply some kind of object-oriented design pattern, like > > Builder, here? One huge builder with multiple child builders. > > > > I feel that too many `if-then-else` and too long methods are disturbing > > beyond readability / comprehension. > > > > I also would separate out different responsibilities to different, > > maybe nested, classes - and test them separately. I simplified the V2SLAServet class, it does not deal with the filter fields directly anymore. Adding a new field will be possible without modifing this class. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java > > Lines 136-141 (original), 153-175 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068369#file2068369line154> > > > > This looks pretty terrible, again. What about applying some design > > pattern / restructuring the code? I removed this whole part, this was not too useful. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java > > Lines 144-147 (original), 178-198 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068369#file2068369line179> > > > > This looks pretty terrible, again. What about applying some design > > pattern / restructuring the code? I also removed this part. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java > > Line 16 (original), 16 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068370#file2068370line16> > > > > Please revert this change. Reverted. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java > > Line 19 (original), 19 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068372#file2068372line19> > > > > Please revert this change. Reverted. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/servlet/TestV2SLAServlet.java > > Lines 111-115 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068373#file2068373line111> > > > > Please extract to separate test methods to provide an even more > > maintainable piece of code :) Even more? :) I separated the test methods. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > docs/src/site/twiki/DG_SLAMonitoring.twiki > > Lines 173-182 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068374#file2068374line173> > > > > We'd need also `curl` based examples with sample JSON output for better > > user insight. I've added samples similar to the already existing samples. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > release-log.txt > > Lines 3-4 (original) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068375#file2068375line3> > > > > Please revert this change. Reverted. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > webapp/src/main/webapp/console/sla/js/oozie-sla.js > > Line 29 (original), 29 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068377#file2068377line29> > > > > Any chance to restructure? Too many `if-then-else` without providing > > readable code. I replaced the if-then-else block with a for cycle. Only kept a couple of ifs because of the special handling of some of the fields. > On Aug. 6, 2018, 12:14 p.m., András Piros wrote: > > webapp/src/main/webapp/console/sla/js/oozie-sla.js > > Lines 30-35 (original), 30-54 (patched) > > <https://reviews.apache.org/r/68140/diff/2/?file=2068377#file2068377line30> > > > > Any chance to restructure? Eliminated most of the fields, only kept 2 because of the special handling. - Andras ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68140/#review206880 ----------------------------------------------------------- On Aug. 14, 2018, 9:22 a.m., Andras Salamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68140/ > ----------------------------------------------------------- > > (Updated Aug. 14, 2018, 9:22 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 949b4532 > > core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java > b54161e9 > core/src/main/java/org/apache/oozie/executor/jpa/sla/SelectQuery.java > PRE-CREATION > 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/executor/jpa/sla/TestSelectQuery.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/3/ > > > Testing > ------- > > > Thanks, > > Andras Salamon > >
