> 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)
wandering if the list increases beyond these two. also alert-contact is email. right? can people give multiple emails? > 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 ok. > 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 can user give anything like this key=;? > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionCheckXCommand.java, > > line 53 > > <https://reviews.apache.org/r/10569/diff/7/?file=292830#file292830line53> > > > > deprecated ? what will be the replacement > > Mona Chitnis wrote: > This is not deprecated. It suppressed the javac warnings thrown due to > presence of deprecated classes and methods (deprecated SLAEventBean for > backward compatibility) got it. > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionMaterializeCommand.java, > > line 283 > > <https://reviews.apache.org/r/10569/diff/7/?file=292832#file292832line283> > > > > is it writing to SLA_CALCULATION table that will be removed in another > > JIRA? > > Mona Chitnis wrote: > yes. In the other JIRA, that writing part will be removed and replaced > with different logic in 1 table (SLA_SUMMARY) itself ok > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordKillXCommand.java, > > line 177 > > <https://reviews.apache.org/r/10569/diff/7/?file=292836#file292836line177> > > > > just to mention. If line 172 succeeds but some exception happened > > between 173-181, there is no way to rollback. > > it is ok for the time being! > > Mona Chitnis wrote: > I see. If 172 succeeds - persisting to DB, that is good enough as DB will > be ultimate source of truth. The lines 173-181, if exception, will fail in > generating event. In immediate enhancement Jira OOZIE-1379, an event like SLA > End_Miss will only be sent after double-checking against DB not urgent! > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordSuspendXCommand.java, > > line 187 > > <https://reviews.apache.org/r/10569/diff/7/?file=292841#file292841line187> > > > > preformWrite looks similar in Kill and suspend, is there a way to put > > it in one place and reuse? > > Mona Chitnis wrote: > ok. will abstract it into parent class TransitionXCommand. That way later > on other commands like CoordRerunX, PauseTransitionX, ResumeTransitionX etc > can also utilize it if event handling is enabled for them ok > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordinatorXCommand.java, > > line 61 > > <https://reviews.apache.org/r/10569/diff/7/?file=292842#file292842line61> > > > > Null checkings are not required? > > Mona Chitnis wrote: > Since this utility method is called from say a Coord Action XCommand, the > coordAction object will never be null. The XCommand would have checked in > loadState() and thrown exception much before itself. ok. > 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. so jpa service keeps a transaction concepts? I thought each execute() persist the data and the idea of transaction are done at that stage. > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/command/wf/SubmitXCommand.java, > > line 283 > > <https://reviews.apache.org/r/10569/diff/7/?file=292848#file292848line283> > > > > Why do we need to create action ID here? It is already generated in > > another place. Wandering if we are creating multiple ids for one action. > > Mona Chitnis wrote: > good caution noted here. But the method that is "generating" the childId > here, is not actually any auto-increment generation but simply appending > action-name to workflow job-id > id = ParamChecker.notEmpty(id, "id") + "@" + > ParamChecker.notEmpty(childName, "childName"); > > So it will return the same id each time. Method is slightly misnomer here > but not changing it as its unrelated to this JIRA got it. wandering if there is any 'actionId' already created could be reused. it's ok. > 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 new reason given here is not an issue. if you're strong about the first reason mentioned above, than it's ok. > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/event/WorkflowActionEvent.java, > > line 58 > > <https://reviews.apache.org/r/10569/diff/7/?file=292856#file292856line58> > > > > Does "DONE" means "SUCCESS"? > > Please double check it. > > Mona Chitnis wrote: > Checked. For workflow actions specifically the success statuses can be OK > or DONE. Nothing called SUCCEEDED there. if i remember correctly, DONE is not a terminal state. Another command such as ActionEnvCommand put "DONE" --> OK. double check pls. > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > line 112 > > <https://reviews.apache.org/r/10569/diff/7/?file=292869#file292869line112> > > > > is it possible to send more specific Exception instead of basic > > Exception. Trie for other places. > > Mona Chitnis wrote: > its tricky to limit to specific exceptions as the listener > implementations extending either JobEventListener or SLAEventListener are > user-pluggable, and can throw a variety of exceptions e.g. jms exception, > address exception (if email) and others ok. > 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. can you please address this? > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java, > > line 47 > > <https://reviews.apache.org/r/10569/diff/7/?file=292879#file292879line47> > > > > All new classes, please add some comments at least in class level. what > > is does. who called this class. > > Mona Chitnis wrote: > okay adding them ok. > 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 I thought we decided to give it in relative sense. Now thinking more on this. In some cases, NT could be passed (catch-up). > 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 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. > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java, > > line 167 > > <https://reviews.apache.org/r/10569/diff/7/?file=292879#file292879line167> > > > > what will be in "else" block? > > Mona Chitnis wrote: > overflow handling if a different implementation can do so. Adding a LOG > line here Add LOG.warning... > 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? Please comment on this. - Mohammad ----------------------------------------------------------- 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 > >
