> On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/event/CoordinatorActionEvent.java, > > line 32 > > <https://reviews.apache.org/r/9602/diff/7/?file=276751#file276751line32> > > > > remove those code , if not needed in future.
I have a use-case in mind for this being a useful metric in the metric. Say the user ingests these notifications for some post-analysis on his coordinators, finding trends in his jobs and such. Then knowing frequency would be useful. So keeping it there for now > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/event/CoordinatorJobEvent.java, > > line 62 > > <https://reviews.apache.org/r/9602/diff/7/?file=276752#file276752line62> > > > > How the user will know if it is DOENWITH ERROR or KEILLED? Any message > > text associated with that could be helpful. the line above the switch case ladder sets the "actual" status, i.e. DONEWITHERROR, KILLED etc. User will know from the 'status' field. 'eventStatus' is meant to be the coarse version > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/event/MemoryEventQueue.java, line > > 59 > > <https://reviews.apache.org/r/9602/diff/7/?file=276754#file276754line59> > > > > should be at least WARN. done > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/event/MemoryEventQueue.java, line > > 63 > > <https://reviews.apache.org/r/9602/diff/7/?file=276754#file276754line63> > > > > WARN. better error handling. please check how it is handled in > > CallableQueuService done. changed to warn > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/event/MemoryEventQueue.java, line > > 87 > > <https://reviews.apache.org/r/9602/diff/7/?file=276754#file276754line87> > > > > use the pattern like > > else { > > break > > }. > > > > Also a LOG info/debug to define why it reached here. done > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/event/WorkflowActionEvent.java, > > line 62 > > <https://reviews.apache.org/r/9602/diff/7/?file=276755#file276755line62> > > > > Do we need to handle START_MANUAL or END_MANUAL? > > Because user needs to know they need to intervene. Had removed it in this patch, but I agree its a good idea to keep it. Will add the *_MANUAL statuses in the patch, and I believe they'd map to WORKFLOW_ACTION_SUSPEND event type. Note however, event generation in WF Action commands will be done as a separate JIRA > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > line 75 > > <https://reviews.apache.org/r/9602/diff/7/?file=276760#file276760line75> > > > > Very long method. If possible, modularize into multiple method. done > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > line 85 > > <https://reviews.apache.org/r/9602/diff/7/?file=276760#file276760line85> > > > > is it possible, 'listernerclass' could be 'null'. in that case add > > "listernerClass != null && i <linsternerClass.lentgth) Will be adding a default implementation for the listener (part of the other JIRA for JMS listener), in which case 'listenerclass' wont be null > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > line 91 > > <https://reviews.apache.org/r/9602/diff/7/?file=276760#file276760line91> > > > > Is this a right approach? Just report the failure. shoudl it fail the > > whole service? > > I'm sure also. Do you? > > The error could arise from a mistyped or non-existent class in the config properties. We wouldnt want the whole service to fail in that case. There will be a default listener anyway > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > line 119 > > <https://reviews.apache.org/r/9602/diff/7/?file=276760#file276760line119> > > > > what does it mean? WF_JOB and COORD_ACTION is must? > > > > If someone wants *only* COORD_JOB. How will it be achieved? The rationale was that WF_JOB and COORD_ACTION would be the most common use-case. In this case *only* COORD_JOB is not possible. But on the receiving end, user can filter incoming messages on app-type COORD_JOB and it is achieved. > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > line 121 > > <https://reviews.apache.org/r/9602/diff/7/?file=276760#file276760line121> > > > > so WFJOB could be twice. right? one from default and another from > > config. > > Please rethink about this behavior. Hence using a HashSet. It will eliminate duplicates. > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > lines 124-125 > > <https://reviews.apache.org/r/9602/diff/7/?file=276760#file276760line124> > > > > Default '0' is correct? good catch. changing it to '5' - consistent with the default in SchedulerService init > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > line 208 > > <https://reviews.apache.org/r/9602/diff/7/?file=276760#file276760line208> > > > > For better modularization, make it a separate method something like > > runJob() and the else part as runSLA() done > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > line 226 > > <https://reviews.apache.org/r/9602/diff/7/?file=276760#file276760line226> > > > > Add the jobType in the message done > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > line 251 > > <https://reviews.apache.org/r/9602/diff/7/?file=276760#file276760line251> > > > > Same done > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java, > > line 273 > > <https://reviews.apache.org/r/9602/diff/7/?file=276760#file276760line273> > > > > same... done > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/event/listener/DummyJobEventListener.java, > > line 2 > > <https://reviews.apache.org/r/9602/diff/7/?file=276757#file276757line2> > > > > Why do we need it? for unit-testing? moving it as the testcase's inner class > On April 1, 2013, 4:06 a.m., Mohammad Islam wrote: > > trunk/core/src/main/java/org/apache/oozie/command/wf/SuspendXCommand.java, > > line 50 > > <https://reviews.apache.org/r/9602/diff/7/?file=276748#file276748line50> > > > > will it be exposed to user? if yes, than it is better to define it on > > the client package. I didn't get this. The variable is simply to avoid using strings in the method invocations here. This is not exposed to user directly, only through the event's error message - Mona ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9602/#review18570 ----------------------------------------------------------- On March 31, 2013, 6:07 p.m., Mona Chitnis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9602/ > ----------------------------------------------------------- > > (Updated March 31, 2013, 6:07 p.m.) > > > Review request for oozie. > > > Description > ------- > > https://issues.apache.org/jira/browse/OOZIE-1209 > WIP patch > > > This addresses bug OOZIE-1209. > https://issues.apache.org/jira/browse/OOZIE-1209 > > > Diffs > ----- > > trunk/client/src/main/java/org/apache/oozie/client/SLAEvent.java 1462882 > trunk/client/src/main/java/org/apache/oozie/client/event/Event.java > PRE-CREATION > trunk/client/src/main/java/org/apache/oozie/client/event/JobEvent.java > PRE-CREATION > trunk/client/src/main/java/org/apache/oozie/client/event/SLAEvent.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 1462882 > > trunk/core/src/main/java/org/apache/oozie/client/rest/JsonCoordinatorAction.java > 1462882 > trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionCheckXCommand.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionInputCheckXCommand.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionReadyXCommand.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionStartXCommand.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionTimeOutXCommand.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordActionUpdateXCommand.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordKillXCommand.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordMaterializeTransitionXCommand.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordPushDependencyCheckXCommand.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordRerunXCommand.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/coord/CoordinatorXCommand.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/command/wf/ActionCheckXCommand.java > 1462882 > trunk/core/src/main/java/org/apache/oozie/command/wf/KillXCommand.java > 1462882 > trunk/core/src/main/java/org/apache/oozie/command/wf/ResumeXCommand.java > 1462882 > trunk/core/src/main/java/org/apache/oozie/command/wf/SignalXCommand.java > 1462882 > trunk/core/src/main/java/org/apache/oozie/command/wf/SuspendXCommand.java > 1462882 > trunk/core/src/main/java/org/apache/oozie/command/wf/WorkflowXCommand.java > 1462882 > trunk/core/src/main/java/org/apache/oozie/event/BundleJobEvent.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/event/CoordinatorActionEvent.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/event/CoordinatorJobEvent.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/event/EventQueue.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/event/MemoryEventQueue.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/event/WorkflowActionEvent.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/event/WorkflowJobEvent.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/event/listener/DummyJobEventListener.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/event/listener/JobEventListener.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/executor/jpa/CoordinatorJobGetForUserAppnameJPAExecutor.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/service/EventHandlerService.java > PRE-CREATION > trunk/core/src/main/java/org/apache/oozie/service/RecoveryService.java > 1462882 > > trunk/core/src/main/java/org/apache/oozie/sla/event/listener/SLAEventListener.java > PRE-CREATION > trunk/core/src/main/resources/oozie-default.xml 1462882 > > trunk/core/src/test/java/org/apache/oozie/command/coord/TestCoordActionStartXCommand.java > 1462882 > trunk/core/src/test/java/org/apache/oozie/event/TestEventGeneration.java > PRE-CREATION > trunk/core/src/test/java/org/apache/oozie/event/TestEventQueue.java > PRE-CREATION > > trunk/core/src/test/java/org/apache/oozie/service/TestEventHandlerService.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/9602/diff/ > > > Testing > ------- > > unit tests added > > > Thanks, > > Mona Chitnis > >
