> 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, 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. > 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. > 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 > >
