> On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/client/rest/sla/JsonSLARegistrationEvent.java, > > line 93 > > <https://reviews.apache.org/r/10569/diff/7/?file=292829#file292829line93> > > > > Will it be of type blob? > > Mona Chitnis wrote: > no. limited length char storage such as varchar2 would suffice, since > these are fields with deterministic lengths (alert-events, alert-contact) > > Mohammad Islam wrote: > wandering if the list increases beyond these two. also alert-contact is > email. right? can people give multiple emails?
Yes key-value list can increase to more later. We are not handling multiple email addresses in the default email listener right now, but seems valid use case. Even with that, we want to strictly avoid using blobs, and think 4K length (varchar2) should be more than sufficient > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/client/rest/sla/JsonSLARegistrationEvent.java, > > line 285 > > <https://reviews.apache.org/r/10569/diff/7/?file=292829#file292829line285> > > > > Not clear what is string format of the config? will there be any "{". > > Mona Chitnis wrote: > The HashMap javadocs specify the toString() implementation to be > {k1=v1},{k2=v2}.. so this function is based on that standard. The starting > brace '{' is removed by the line 'slaConfigMap.put(pair[0].substring(1), > pair[1]);' which takes substring starting after the brace. I don't see us > changing the datastructure from HashMap to something else, which is when this > method slaConfigStringToMap() would need to handle that other implementation > > Mohammad Islam wrote: > ok. In the current patch I have actually written custom function both for converting from map to string and vice versa, for complete control and robustness if java implementations underneath happen to change with jdk versions > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/client/rest/sla/JsonSLARegistrationEvent.java, > > line 289 > > <https://reviews.apache.org/r/10569/diff/7/?file=292829#file292829line289> > > > > check if pair.length == 2. otherwise NPE could occurs. > > Mona Chitnis wrote: > The key=value string here is internally constructed by Oozie itself so it > WILL have pair.length=2, but adding a check just to avoid any chance of NPE > > Mohammad Islam wrote: > can user give anything like this key=;? for <alert-events> the assumption is 'comma' separated list. I will add some input sanitizing check if user gives ";" as value > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > line 245 > > <https://reviews.apache.org/r/10569/diff/7/?file=292869#file292869line245> > > > > For "any" reason, if msgType is NOT JOB or SLA, this thing will be an > > infinite loop. So call iter.next() once outside if. > > Mohammad Islam wrote: > can you please address this? this has been addressed in new patch > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/sla/SLACalculatorBean.java, line > > 34 > > <https://reviews.apache.org/r/10569/diff/7/?file=292878#file292878line34> > > > > will be removed ultimately? yes > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java, > > line 103 > > <https://reviews.apache.org/r/10569/diff/7/?file=292879#file292879line103> > > > > So we assume, all times are in UTC. right? > > Also user will provide in absolute or relative to NT. > > Mona Chitnis wrote: > yes you are right that is the assumption. If turns out wrong, can address > later > > Mohammad Islam wrote: > I thought we decided to give it in relative sense. > Now thinking more on this. In some cases, NT could be passed (catch-up). > User gives the <should-start> value in relative sense - num(minutes/hours) w.r.t. NominalTime. The Registration bean here calculates and stores NT + should-start = Expected-Start time <-- absolute. That getter getExpectedStart gives this calculated date. I think I want to address catchup mode separately. There can be multiple permutations based on use-case (need some input from users). E.g. is catchup understood as a miss-event? If not, we can reset the NT in catchup mode for SLA purposes to the time that the coord Job actually started materializing actions. > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java, > > line 123 > > <https://reviews.apache.org/r/10569/diff/7/?file=292879#file292879line123> > > > > if expected start/end time is null. NPE? > > Mona Chitnis wrote: > added handling for expected start is null following your comment about > <should-start> being optional. Then expected end cannot be null since > <should-end> we are keeping mandatory. If the parse operation on the user > provided value fails, an exception will be thrown in registration phase > itself, failing the submit command > > Mohammad Islam wrote: > ok. So user can't give something like, my 'duration' should not be more > than 2 hours. Only to catch the run-away jobs. Not caring any start or end. > > yes. Since we are making nominal-time mandatory, makes sense to make should-end mandatory and use a 'default' duration from these, rather than keeping all optional. If not, and user only gives <duration> , even nominal-time doesn't hold good for that job. > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/sla/SLAOperations.java, line 139 > > <https://reviews.apache.org/r/10569/diff/7/?file=292880#file292880line139> > > > > can we check at the beginning of this method and return if disabled? > > Mohammad Islam wrote: > Please comment on this. this has been addressed in new patch > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/command/wf/WorkflowXCommand.java, > > line 60 > > <https://reviews.apache.org/r/10569/diff/7/?file=292850#file292850line60> > > > > As mentioned before, in place of passing "em" as arguments, can't we > > get JPAService.getEM() in EventService.queueEvent()? No need to pass the > > same argument is all the places. > > > > Mona Chitnis wrote: > same comment as above. Plus dont want to incur the latency of additional > Services.get.get(JPAService) operation when we can pass the reference to > existing object as argument. Let me know if this is still an issue > > Mohammad Islam wrote: > new reason given here is not an issue. if you're strong about the first > reason mentioned above, than it's ok. yes..reconsidering this > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/command/wf/SignalXCommand.java, > > line 322 > > <https://reviews.apache.org/r/10569/diff/7/?file=292847#file292847line322> > > > > all places, we are passing the EM as a new param. Can't it be get > > within genenrateEvent(..) instead. > > Mona Chitnis wrote: > Later on, when Events become persistent for reliability, the event should > be persisted in the same transaction as the caller XCommand persisting the > wf/coord bean to DB. Hence passing the EM. This was one of Tucu's review > comments. would have to pass one of either EM or reference to jpaService > anyway, to achieve this. > > Mohammad Islam wrote: > so jpa service keeps a transaction concepts? I thought each execute() > persist the data and the idea of transaction are done at that stage. > you are right. Inside JPAService.execute() method, there is a call to EntityManagerFactory.createEntityManager() which creates a new EM context each time, ignoring the fact that we may have an existing context. In this case, I think it makes sense to remove passing the EM argument - Mona ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10569/#review20638 ----------------------------------------------------------- On May 16, 2013, 10:36 p.m., Mona Chitnis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10569/ > ----------------------------------------------------------- > > (Updated May 16, 2013, 10:36 p.m.) > > > Review request for oozie. > > > Description > ------- > > 1. Revisiting the SLA handling in Oozie > 2. Addition of a calculator service to process sla events in a continuous > fashion > 3. Added new oozie-sla schema v0.2, concise and relevant > > > This addresses bug OOZIE-1244. > https://issues.apache.org/jira/browse/OOZIE-1244 > > > Diffs > ----- > > trunk/client/src/main/java/org/apache/oozie/AppType.java PRE-CREATION > trunk/client/src/main/java/org/apache/oozie/cli/OozieCLI.java 1483581 > trunk/client/src/main/java/org/apache/oozie/client/JMSConnectionInfo.java > 1483581 > trunk/client/src/main/java/org/apache/oozie/client/SLAEvent.java 1483581 > trunk/client/src/main/java/org/apache/oozie/client/event/Event.java 1483581 > trunk/client/src/main/java/org/apache/oozie/client/event/JobEvent.java > 1483581 > trunk/client/src/main/java/org/apache/oozie/client/event/SLAEvent.java > 1483581 > > trunk/client/src/main/java/org/apache/oozie/client/event/jms/JSONMessageDeserializer.java > 1482602 > > trunk/client/src/main/java/org/apache/oozie/client/event/jms/MessageDeserializer.java > 1482602 > > trunk/client/src/main/java/org/apache/oozie/client/event/message/CoordinatorActionMessage.java > 1482171 > > trunk/client/src/main/java/org/apache/oozie/client/event/message/EventMessage.java > 1482171 > > trunk/client/src/main/java/org/apache/oozie/client/event/message/JobMessage.java > 1482171 > > trunk/client/src/main/java/org/apache/oozie/client/event/message/WorkflowJobMessage.java > 1482171 > trunk/client/src/main/java/org/apache/oozie/client/rest/JsonToBean.java > 1483581 > trunk/client/src/main/resources/oozie-coordinator-0.4.xsd 1483581 > trunk/client/src/main/resources/oozie-sla-0.2.xsd PRE-CREATION > trunk/client/src/main/resources/oozie-workflow-0.4.5.xsd 1483581 > trunk/client/src/main/resources/oozie-workflow-0.5.xsd PRE-CREATION > trunk/client/src/test/java/org/apache/oozie/client/rest/TestJsonToBean.java > 1483581 > trunk/core/pom.xml 1483581 > trunk/core/src/main/conf/oozie-site.xml 1483581 > trunk/core/src/main/java/org/apache/oozie/CoordinatorActionBean.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/ErrorCode.java 1483581 > trunk/core/src/main/java/org/apache/oozie/SLAEventBean.java 1483581 > trunk/core/src/main/java/org/apache/oozie/client/rest/JsonSLAEvent.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/client/rest/sla/JsonSLARegistrationEvent.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/command/TransitionXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionCheckXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionMaterializeCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionStartXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionTimeOutXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionUpdateXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordKillXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordMaterializeTransitionXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordPushDependencyCheckXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordRerunXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordSubmitXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordSuspendXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordinatorXCommand.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/command/coord/SLAEventsXCommand.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/command/wf/ActionEndXCommand.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/command/wf/KillXCommand.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/command/wf/ResumeXCommand.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/command/wf/SignalXCommand.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/command/wf/SubmitXCommand.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/command/wf/SuspendXCommand.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/command/wf/WorkflowXCommand.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/event/BundleJobEvent.java 1483581 > trunk/core/src/main/java/org/apache/oozie/event/CoordinatorActionEvent.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/event/CoordinatorJobEvent.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/event/EventQueue.java 1483581 > trunk/core/src/main/java/org/apache/oozie/event/MemoryEventQueue.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/event/WorkflowActionEvent.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/event/WorkflowJobEvent.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/event/listener/JobEventListener.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsNotCompletedJPAExecutor.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/SLAEventInsertJPAExecutor.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/SLAEventsGetForFilterJPAExecutor.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/SLAEventsGetForSeqIdJPAExecutor.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/SLAEventsGetJPAExecutor.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/sla/SLACalculationInsertUpdateJPAExecutor.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/sla/SLACalculatorGetJPAExecutor.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/sla/SLARegistrationGetJPAExecutor.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/sla/SLASummaryGetJPAExecutor.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/jms/JMSJobEventListener.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/service/JMSTopicService.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/service/JPAService.java 1483581 > trunk/core/src/main/java/org/apache/oozie/service/SchedulerService.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/service/SchemaService.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/servlet/SLAServlet.java 1483581 > trunk/core/src/main/java/org/apache/oozie/sla/SLACalcStatus.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/sla/SLACalculator.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/sla/SLACalculatorBean.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/sla/SLAOperations.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/sla/SLARegistrationBean.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/sla/SLASummaryBean.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/sla/event/listener/SLAEventListener.java > 1483581 > > trunk/core/src/main/java/org/apache/oozie/sla/listener/SLAEventListener.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/sla/listener/SLAJobEventListener.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/sla/service/SLAService.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/store/SLAStore.java 1483581 > trunk/core/src/main/java/org/apache/oozie/util/XmlUtils.java 1483581 > trunk/core/src/main/java/org/apache/oozie/util/db/SLADbOperations.java > 1483581 > trunk/core/src/main/java/org/apache/oozie/util/db/SLADbXOperations.java > 1483581 > trunk/core/src/main/resources/META-INF/persistence.xml 1483581 > trunk/core/src/main/resources/oozie-default.xml 1483581 > trunk/core/src/test/java/org/apache/oozie/TestSLAEventBean.java 1483581 > > trunk/core/src/test/java/org/apache/oozie/command/coord/TestCoordActionMaterializeCommand.java > 1483581 > > trunk/core/src/test/java/org/apache/oozie/command/coord/TestCoordMaterializeTransitionXCommand.java > 1483581 > trunk/core/src/test/java/org/apache/oozie/event/TestEventGeneration.java > 1483581 > trunk/core/src/test/java/org/apache/oozie/event/TestEventQueue.java 1483581 > > trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestSLAEventsGetJPAExecutor.java > 1483581 > trunk/core/src/test/java/org/apache/oozie/jms/TestJMSJobEventListener.java > 1483581 > > trunk/core/src/test/java/org/apache/oozie/service/TestEventHandlerService.java > 1483581 > trunk/core/src/test/java/org/apache/oozie/service/TestJMSTopicService.java > 1483581 > > trunk/core/src/test/java/org/apache/oozie/sla/TestSLACalculationJPAExecutor.java > PRE-CREATION > trunk/core/src/test/java/org/apache/oozie/sla/TestSLAEventGeneration.java > PRE-CREATION > trunk/core/src/test/java/org/apache/oozie/sla/TestSLAJobEventListener.java > PRE-CREATION > > trunk/core/src/test/java/org/apache/oozie/sla/TestSLARegistrationGetJPAExecutor.java > PRE-CREATION > trunk/core/src/test/java/org/apache/oozie/sla/TestSLAService.java > PRE-CREATION > trunk/core/src/test/java/org/apache/oozie/test/XDataTestCase.java 1483581 > trunk/core/src/test/java/org/apache/oozie/test/XTestCase.java 1483581 > trunk/core/src/test/resources/coord-action-sla.xml PRE-CREATION > trunk/core/src/test/resources/wf-action-sla.xml PRE-CREATION > trunk/core/src/test/resources/wf-job-sla.xml PRE-CREATION > trunk/examples/src/main/apps/sla/coordinator.xml 1483581 > trunk/examples/src/main/apps/sla/job.properties 1483581 > trunk/examples/src/main/apps/sla/workflow.xml 1483581 > trunk/tools/src/main/java/org/apache/oozie/tools/OozieDBCLI.java 1483581 > > Diff: https://reviews.apache.org/r/10569/diff/ > > > Testing > ------- > > ongoing > > > Thanks, > > Mona Chitnis > >
