On 03/07/13 9:02 PM, "Ryan Dietrich" <r...@betterservers.com> wrote:
> > >> On July 3, 2013, 9:04 a.m., Murali Reddy wrote: >> > server/src/com/cloud/async/AsyncJobManagerImpl.java, lines 147-156 >> > >><https://reviews.apache.org/r/12223/diff/3/?file=314993#file314993line147 >>> >> > >> > IMO this is not right. event type in asyn job are used for >>'action events' which are basically user actions. Since we are just >>dealing with 'asyn job' events its more appropriate events would be like >>schedule of a new job, completion of job, status update of job etc. >>Ofcourse you are adding details like instance/entity type and its UUID, >>and command type etc which makes this event meaningful any way. > >It's the event type OF the command that the async job is performing. >It's part of the abstract class that you're forced to implement when >deriving from BaseAsyncCmd! Not having it would be a mistake, My concern was this event category becoming redundant of action events. I would imagine use case for asyn job events, would be to fire an CloudStack API which is asyn, and instead of polling management server for job status be able to get the callback when job is done. Since you have already added the job status (submit/complete/update) as event type for this async job event category I am fine with the patch you submitted. Applied it on both master and 4.2. > I thought you'd comment on the fact that I'm substring-ing it out of >JSON instead of just creating a new database column within AsyncJobVO. I guess current approach Is fine. Event type is not primary attribute of Async Job. > > >> On July 3, 2013, 9:04 a.m., Murali Reddy wrote: >> > server/src/com/cloud/async/AsyncJobManagerImpl.java, line 161 >> > >><https://reviews.apache.org/r/12223/diff/3/?file=314993#file314993line161 >>> >> > >> > Like mentioned in above comment, it would be appropriate we >>capture schedule/complete/status update of async job here > >No problem. I'll add an argument to the publish function. It's called >in three places. Once for submit, once for update and once for complete. > It'll be a simple string argument that will be captured and sent within >the event description. > > >> On July 3, 2013, 9:04 a.m., Murali Reddy wrote: >> > server/src/com/cloud/async/AsyncJobManagerImpl.java, line 162 >> > >><https://reviews.apache.org/r/12223/diff/3/?file=314993#file314993line162 >>> >> > >> > any way we have instance id and instance UUID, please use them to >>form routing key. > >But what if we don't? Are you saying to make getInstanceId / >getInstanceType abstract, forcing all async commands to implement those >function? Right now they have null returning definitions, which makes >this line of code make sense. Yeah, I noticed not all commands have implemented, probability need an effort to clean it up and make the methods abstract. Can you please file a bug for this? > > >> On July 3, 2013, 9:04 a.m., Murali Reddy wrote: >> > server/src/com/cloud/async/AsyncJobManagerImpl.java, line 173 >> > >><https://reviews.apache.org/r/12223/diff/3/?file=314993#file314993line173 >>> >> > >> > Internal id's are not exposed. Since UUID is populated in the >>description that should be enough. > >Cool, I'll make that change. > > >- Ryan > > >----------------------------------------------------------- >This is an automatically generated e-mail. To reply, visit: >https://reviews.apache.org/r/12223/#review22704 >----------------------------------------------------------- > > >On July 3, 2013, 2:17 p.m., Ryan Dietrich wrote: >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/12223/ >> ----------------------------------------------------------- >> >> (Updated July 3, 2013, 2:17 p.m.) >> >> >> Review request for cloudstack, Marcus Sorensen and Murali Reddy. >> >> >> Bugs: CLOUDSTACK-3190 >> >> >> Repository: cloudstack-git >> >> >> Description >> ------- >> >> Updated AsyncJobManagerImpl to publish async job events when async jobs >>are created, updated and completed. >> I am currently stashing the command event description in the >>commandInfo structure, and then pulling it back out as needed. >> I could switch this to make a database change, but that seemed like a >>more invasive change. >> >> I have further diffs to clean up ActionEvent and AlertEvent as well. >> >> >> Diffs >> ----- >> >> api/src/com/cloud/event/EventCategory.java cee6529 >> server/src/com/cloud/api/ApiServer.java 0cd1d61 >> server/src/com/cloud/async/AsyncJobManagerImpl.java 0101a8a >> >> Diff: https://reviews.apache.org/r/12223/diff/ >> >> >> Testing >> ------- >> >> Manual testing only at this point. I am more than willing to write a >>python test using marvin, but I'm unsure if marvin has rabbitmq library >>support or not yet. Please advise. >> >> >> Thanks, >> >> Ryan Dietrich >> >> > >
