----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24487/#review61210 -----------------------------------------------------------
Core logic looks great, few comments. 1. We are only supporting coord_name with bundle, it will be nice if we can support coord_id as well. 2. I guess we are not displaying sla alert information anywhere, it will be hard for user to know which action is disabled. I think it will be useful if we can display sla enable/disable information with some command and in Oozie UI SLA Tab. client/src/main/java/org/apache/oozie/cli/OozieCLI.java <https://reviews.apache.org/r/24487/#comment102732> Has args should be true. SLA_DISABLE_ALERT need jobId. client/src/main/java/org/apache/oozie/cli/OozieCLI.java <https://reviews.apache.org/r/24487/#comment102733> Has args should be true. client/src/main/java/org/apache/oozie/cli/OozieCLI.java <https://reviews.apache.org/r/24487/#comment102734> Has args should be true. client/src/main/java/org/apache/oozie/cli/OozieCLI.java <https://reviews.apache.org/r/24487/#comment102735> Is this used? client/src/main/java/org/apache/oozie/cli/OozieCLI.java <https://reviews.apache.org/r/24487/#comment102736> Why optionsGroups? They should be added as option, like run, dryrun. client/src/main/java/org/apache/oozie/client/OozieClient.java <https://reviews.apache.org/r/24487/#comment102737> Why not json responce, All oozie webservice responce is json. client/src/main/java/org/apache/oozie/client/OozieClient.java <https://reviews.apache.org/r/24487/#comment102740> Not used anywhere. core/src/main/java/org/apache/oozie/CoordinatorActionBean.java <https://reviews.apache.org/r/24487/#comment104216> Should include ignore status core/src/main/java/org/apache/oozie/CoordinatorActionBean.java <https://reviews.apache.org/r/24487/#comment104217> Should include ignore status core/src/main/java/org/apache/oozie/coord/CoordUtils.java <https://reviews.apache.org/r/24487/#comment104226> Will this work if job freqency is in different time zone? core/src/main/java/org/apache/oozie/coord/CoordUtils.java <https://reviews.apache.org/r/24487/#comment104220> why name cabs? core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java <https://reviews.apache.org/r/24487/#comment104199> Why? I don't think it's being used anywhere. core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java <https://reviews.apache.org/r/24487/#comment104201> Not supported in v0 core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java <https://reviews.apache.org/r/24487/#comment104202> Not supported in v0 core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java <https://reviews.apache.org/r/24487/#comment104203> Not supported in v0 core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java <https://reviews.apache.org/r/24487/#comment104207> This code used in three function, please move it to one sun-function(). core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java <https://reviews.apache.org/r/24487/#comment104251> Since we have diffent command for different job_type, this logic can move to <jobType>XCommand. core/src/main/java/org/apache/oozie/sla/BundleChangeSlaXCommand.java <https://reviews.apache.org/r/24487/#comment104228> BundleChangeSlaXCommand and CoordChangeSlaXCommand does same thing, in that case we can have only one class ChangeSlaXCommand. core/src/main/java/org/apache/oozie/sla/BundleChangeSlaXCommand.java <https://reviews.apache.org/r/24487/#comment102743> Why requeue? core/src/main/java/org/apache/oozie/sla/BundleDisableSlaAlertsXCommand.java <https://reviews.apache.org/r/24487/#comment104230> we can merge BundleDisableSlaAlertsXCommand and CoordDisableSlaAlertsXCommand to DisableSlaAlertsXCommand. core/src/main/java/org/apache/oozie/sla/BundleDisableSlaAlertsXCommand.java <https://reviews.apache.org/r/24487/#comment102744> Do u need to load bundle bean? I guess we are not using it. core/src/main/resources/oozie-default.xml <https://reviews.apache.org/r/24487/#comment104231> Is this used anywhere? core/src/test/java/org/apache/oozie/command/coord/TestCoordSubmitXCommand.java <https://reviews.apache.org/r/24487/#comment104232> why sleep of 5 min. If you need it add it to specific testcase, it will slowdown the testcase execution. - Purshotam Shah On Oct. 20, 2014, 11:20 p.m., Mona Chitnis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24487/ > ----------------------------------------------------------- > > (Updated Oct. 20, 2014, 11:20 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-1913 > https://issues.apache.org/jira/browse/OOZIE-1913 > > > Repository: oozie-git > > > Description > ------- > > See Jira > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/cli/OozieCLI.java 9c2d14b > client/src/main/java/org/apache/oozie/client/OozieClient.java 5e53a18 > > client/src/main/java/org/apache/oozie/client/event/jms/JMSHeaderConstants.java > 801ad7e > client/src/main/java/org/apache/oozie/client/rest/RestConstants.java > 4cc6606 > core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 759e643 > core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 2362084 > core/src/main/java/org/apache/oozie/command/SubmitTransitionXCommand.java > 070cee5 > > core/src/main/java/org/apache/oozie/command/bundle/BundleSubmitXCommand.java > de78ab7 > > core/src/main/java/org/apache/oozie/command/coord/CoordMaterializeTransitionXCommand.java > 05b7a62 > core/src/main/java/org/apache/oozie/coord/CoordUtils.java 4643d73 > > core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java > e6ab09b > core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java > 4bccef4 > core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java c19839f > > core/src/main/java/org/apache/oozie/service/CoordMaterializeTriggerService.java > ee1085a > core/src/main/java/org/apache/oozie/service/EventHandlerService.java > 244c048 > core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java c94d1e2 > core/src/main/java/org/apache/oozie/servlet/SLAServlet.java 2578e41 > core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java b160b46 > core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 8dc9608 > core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java da81b49 > core/src/main/java/org/apache/oozie/sla/BundleChangeSlaXCommand.java > PRE-CREATION > core/src/main/java/org/apache/oozie/sla/BundleDisableSlaAlertsXCommand.java > PRE-CREATION > core/src/main/java/org/apache/oozie/sla/BundleEnableSlaAlertsXCommand.java > PRE-CREATION > core/src/main/java/org/apache/oozie/sla/CoordChangeSlaXCommand.java > PRE-CREATION > core/src/main/java/org/apache/oozie/sla/CoordDisableSlaAlertsXCommand.java > PRE-CREATION > core/src/main/java/org/apache/oozie/sla/CoordEnableSlaAlertsXCommand.java > PRE-CREATION > core/src/main/java/org/apache/oozie/sla/SLACalcStatus.java 189d5ea > core/src/main/java/org/apache/oozie/sla/SLACalculator.java 20f93b5 > core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java 188144e > core/src/main/java/org/apache/oozie/sla/SLAOperations.java f5fc826 > core/src/main/java/org/apache/oozie/sla/service/SLAService.java 89615bc > core/src/main/java/org/apache/oozie/util/CoordActionsInDateRange.java > 7c2620c > core/src/main/resources/oozie-default.xml 26eb7e0 > > core/src/test/java/org/apache/oozie/command/coord/TestCoordSubmitXCommand.java > f13e48f > core/src/test/java/org/apache/oozie/coord/TestCoordUtils.java ae3f18d > core/src/test/java/org/apache/oozie/jms/TestJMSSLAEventListener.java > 30fd151 > core/src/test/java/org/apache/oozie/servlet/DagServletTestCase.java 48193c7 > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 > core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java > db3f6eb > core/src/test/java/org/apache/oozie/store/TestCoordinatorStore.java b8b2405 > > Diff: https://reviews.apache.org/r/24487/diff/ > > > Testing > ------- > > unit tests added, e-2-e test with CLI command done > > > Thanks, > > Mona Chitnis > >
