----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68140/#review206880 -----------------------------------------------------------
client/src/main/java/org/apache/oozie/client/OozieClient.java Lines 177-204 (patched) <https://reviews.apache.org/r/68140/#comment289991> Can you please structure that a bit more? Like having not `String` but `enum` instances, having a nested wrapper class... core/src/main/conf/action-conf/hive.xml Line 28 (original), 28 (patched) <https://reviews.apache.org/r/68140/#comment289985> Please revert this change. core/src/main/java/org/apache/oozie/action/ActionExecutor.java Line 606 (original), 606 (patched) <https://reviews.apache.org/r/68140/#comment289986> Please revert this change. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Line 1780 (original), 1780 (patched) <https://reviews.apache.org/r/68140/#comment289987> Please revert this change. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 1831-1834 (original) <https://reviews.apache.org/r/68140/#comment289988> Please revert this change. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 51-78 (patched) <https://reviews.apache.org/r/68140/#comment289992> Can we extract that to a state object? Too many fields here, we're far away from Single Responsibility Principle. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Line 66 (original), 91 (patched) <https://reviews.apache.org/r/68140/#comment289994> 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. 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/#comment289993> Please extract method. core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetForFilterJPAExecutor.java Lines 425-426 (patched) <https://reviews.apache.org/r/68140/#comment289995> Here we're changing existing behavior. Any chance to create separate test for the change, plus document that? core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java Lines 69-78 (original), 68-94 (patched) <https://reviews.apache.org/r/68140/#comment289996> This looks pretty terrible, please extract / restructure for better maintainability. core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java Line 115 (original), 131 (patched) <https://reviews.apache.org/r/68140/#comment289999> 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. core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java Lines 136-141 (original), 153-175 (patched) <https://reviews.apache.org/r/68140/#comment289997> This looks pretty terrible, again. What about applying some design pattern / restructuring the code? core/src/main/java/org/apache/oozie/servlet/V2SLAServlet.java Lines 144-147 (original), 178-198 (patched) <https://reviews.apache.org/r/68140/#comment289998> This looks pretty terrible, again. What about applying some design pattern / restructuring the code? core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java Line 16 (original), 16 (patched) <https://reviews.apache.org/r/68140/#comment289989> Please revert this change. core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java Line 19 (original), 19 (patched) <https://reviews.apache.org/r/68140/#comment289990> Please revert this change. core/src/test/java/org/apache/oozie/servlet/TestV2SLAServlet.java Lines 111-115 (patched) <https://reviews.apache.org/r/68140/#comment290000> Please extract to separate test methods to provide an even more maintainable piece of code :) docs/src/site/twiki/DG_SLAMonitoring.twiki Lines 173-182 (patched) <https://reviews.apache.org/r/68140/#comment290001> We'd need also `curl` based examples with sample JSON output for better user insight. release-log.txt Lines 3-4 (original) <https://reviews.apache.org/r/68140/#comment289984> Please revert this change. webapp/src/main/webapp/console/sla/js/oozie-sla.js Line 29 (original), 29 (patched) <https://reviews.apache.org/r/68140/#comment290003> Any chance to restructure? Too many `if-then-else` without providing readable code. webapp/src/main/webapp/console/sla/js/oozie-sla.js Lines 30-35 (original), 30-54 (patched) <https://reviews.apache.org/r/68140/#comment290002> Any chance to restructure? - András Piros On Aug. 6, 2018, 7:35 a.m., Andras Salamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68140/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2018, 7:35 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/conf/action-conf/hive.xml fc02e25b > core/src/main/java/org/apache/oozie/action/ActionExecutor.java 1770b973 > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > 8f0f2440 > > 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/action/hadoop/LauncherMainTester.java > ada7005e > > core/src/test/java/org/apache/oozie/action/hadoop/SleepMapperReducerForTest.java > 846441fd > > core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java > 893405e3 > core/src/test/java/org/apache/oozie/servlet/TestV2SLAServlet.java aa633225 > docs/src/site/twiki/DG_SLAMonitoring.twiki c91c227d > release-log.txt 5100d97d > 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/2/ > > > Testing > ------- > > > Thanks, > > Andras Salamon > >
