> 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?
no. limited length char storage such as varchar2 would suffice, since these are fields with deterministic lengths (alert-events, alert-contact) > 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 "{". 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 > 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. 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 > 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 This is not deprecated. It suppressed the javac warnings thrown due to presence of deprecated classes and methods (deprecated SLAEventBean for backward compatibility) > 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? yes. In the other JIRA, that writing part will be removed and replaced with different logic in 1 table (SLA_SUMMARY) itself > 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! 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 > 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? 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 > 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? 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. > 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. 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. > 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. 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 > 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. > > 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 > 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. Checked. For workflow actions specifically the success statuses can be OK or DONE. Nothing called SUCCEEDED there. > 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. okay adding them > 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. yes you are right that is the assumption. If turns out wrong, can address later > 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? 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 > 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? overflow handling if a different implementation can do so. Adding a LOG line here > On May 16, 2013, 7:54 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/sla/SLAOperations.java, line 185 > > <https://reviews.apache.org/r/10569/diff/7/?file=292880#file292880line185> > > > > Move this deep-copy method to class SLACalcStatus. Because if there is > > any new member added , you will need to modify in one class. > > okay. added copy constructor there > 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. 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 - Mona ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10569/#review20638 ----------------------------------------------------------- On May 16, 2013, 1:23 a.m., Mona Chitnis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10569/ > ----------------------------------------------------------- > > (Updated May 16, 2013, 1:23 a.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 1482602 > trunk/client/src/main/java/org/apache/oozie/client/JMSConnectionInfo.java > 1482602 > trunk/client/src/main/java/org/apache/oozie/client/SLAEvent.java 1482602 > trunk/client/src/main/java/org/apache/oozie/client/event/Event.java 1482602 > trunk/client/src/main/java/org/apache/oozie/client/event/JobEvent.java > 1482602 > trunk/client/src/main/java/org/apache/oozie/client/event/SLAEvent.java > 1482602 > trunk/client/src/main/java/org/apache/oozie/client/rest/JsonToBean.java > 1482602 > trunk/client/src/main/resources/oozie-coordinator-0.4.xsd 1482602 > trunk/client/src/main/resources/oozie-sla-0.2.xsd PRE-CREATION > trunk/client/src/main/resources/oozie-workflow-0.4.5.xsd 1482602 > trunk/client/src/main/resources/oozie-workflow-0.5.xsd PRE-CREATION > trunk/client/src/test/java/org/apache/oozie/client/rest/TestJsonToBean.java > 1482602 > trunk/core/pom.xml 1482602 > trunk/core/src/main/conf/oozie-site.xml 1482602 > trunk/core/src/main/java/org/apache/oozie/CoordinatorActionBean.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/ErrorCode.java 1482602 > trunk/core/src/main/java/org/apache/oozie/SLAEventBean.java 1482602 > trunk/core/src/main/java/org/apache/oozie/client/rest/JsonSLAEvent.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/client/rest/sla/JsonSLARegistrationEvent.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionCheckXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionMaterializeCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionStartXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionTimeOutXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionUpdateXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordKillXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordMaterializeTransitionXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordPushDependencyCheckXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordRerunXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordSubmitXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordSuspendXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordinatorXCommand.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/command/coord/SLAEventsXCommand.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/command/wf/ActionEndXCommand.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/command/wf/KillXCommand.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/command/wf/ResumeXCommand.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/command/wf/SignalXCommand.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/command/wf/SubmitXCommand.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/command/wf/SuspendXCommand.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/command/wf/WorkflowXCommand.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/event/BundleJobEvent.java 1482602 > trunk/core/src/main/java/org/apache/oozie/event/CoordinatorActionEvent.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/event/CoordinatorJobEvent.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/event/EventQueue.java 1482602 > trunk/core/src/main/java/org/apache/oozie/event/MemoryEventQueue.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/event/WorkflowActionEvent.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/event/WorkflowJobEvent.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/event/listener/JobEventListener.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetActionsNotCompletedJPAExecutor.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/SLAEventInsertJPAExecutor.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/SLAEventsGetForFilterJPAExecutor.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/SLAEventsGetForSeqIdJPAExecutor.java > 1482602 > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/SLAEventsGetJPAExecutor.java > 1482602 > > 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 > 1482602 > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/service/JMSTopicService.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/service/JPAService.java 1482602 > trunk/core/src/main/java/org/apache/oozie/service/SchedulerService.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/service/SchemaService.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/service/WorkflowStoreService.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/servlet/SLAServlet.java 1482602 > 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 > 1482602 > > 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 1482602 > trunk/core/src/main/java/org/apache/oozie/util/XmlUtils.java 1482602 > trunk/core/src/main/java/org/apache/oozie/util/db/SLADbOperations.java > 1482602 > trunk/core/src/main/java/org/apache/oozie/util/db/SLADbXOperations.java > 1482602 > trunk/core/src/main/resources/META-INF/persistence.xml 1482602 > trunk/core/src/main/resources/oozie-default.xml 1482602 > > trunk/core/src/test/java/org/apache/oozie/command/coord/TestCoordActionMaterializeCommand.java > 1482602 > > trunk/core/src/test/java/org/apache/oozie/command/coord/TestCoordMaterializeTransitionXCommand.java > 1482602 > trunk/core/src/test/java/org/apache/oozie/event/TestEventGeneration.java > 1482602 > trunk/core/src/test/java/org/apache/oozie/event/TestEventQueue.java 1482602 > > trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestSLAEventsGetJPAExecutor.java > 1482602 > trunk/core/src/test/java/org/apache/oozie/jms/TestJMSJobEventListener.java > 1482602 > > trunk/core/src/test/java/org/apache/oozie/service/TestEventHandlerService.java > 1482602 > trunk/core/src/test/java/org/apache/oozie/service/TestJMSTopicService.java > 1482602 > > 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 1482602 > trunk/core/src/test/java/org/apache/oozie/test/XTestCase.java 1482602 > 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 1482602 > trunk/examples/src/main/apps/sla/job.properties 1482602 > trunk/examples/src/main/apps/sla/workflow.xml 1482602 > trunk/tools/src/main/java/org/apache/oozie/tools/OozieDBCLI.java 1482602 > > Diff: https://reviews.apache.org/r/10569/diff/ > > > Testing > ------- > > ongoing > > > Thanks, > > Mona Chitnis > >
