> On Sept. 12, 2013, 5:28 p.m., Kelven Yang wrote: > > Darren, I saw you added the annotation for group of events and annotated in > > various places, is there any test done for the business logic validation?
I added an annotation @ActionEvents and its used in only one place and that is on AccountManagerImpl.createUserAccount(). In general nested ActionEvents won't work with Spring AOP. So previously createUserAccount had an @ActionEvent and that internally called createUser which also had an @ActionEvent. With Spring AOP the second will not be called. So the @ActionEvents is to specify that two events should be recorded for that method invocation. In general the whole concept of nesting actionevents is somewhat broken. The eventDetails is stored on the CallContext which is a ThreadLocal. There is only one of those, so as you nest the child's details override the parent's. This is how things already were. I don't think the move to Spring AOP will be completely ideal for ActionEvents. It limits some things you can do. But I would argue that this whole ActionEvents framework should be reconcidered. Instead of all this ad-hoc based annotations and random strings of text the developers choose, a simple framework in the API can provide more precise and useful information for auditing. Billing is not based off of ActionEvents and that information is far more accurate and useful too. To further illustrate the complete uselessness (in my opinion) of these events, if you reboot a VM, for example, you get an VM.REBOOT event that has a description "rebooting user vm: 7" What is VM 7? The user has no visibility to internal VM ids. I tested all changes to ensure that the same events were fired as before. There was some exceptions though. In some cases related to the firewall rule stuff you would make one change to a rule that would then fire an event for every single rule you had. In situations like that, since nesting is a problem, I ensured the top level event was still properly recorded. - Darren ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14084/#review26056 ----------------------------------------------------------- On Sept. 11, 2013, 5:07 p.m., Darren Shepherd wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14084/ > ----------------------------------------------------------- > > (Updated Sept. 11, 2013, 5:07 p.m.) > > > Review request for cloudstack, Kelven Yang and Kishan Kavala. > > > Repository: cloudstack-git > > > Description > ------- > > Two fields were added to CallContext to allow call to dynamically change > the event type and description. Additionally a @ActionEvents annotation > was added to allow a method to specify multiple events > > Spring AOP will not intercept calls to "this" so @ActionEvent needs to be > put on public methods that are externally invoked > > Annotations that needed to be changed were identified by doing byte code > analysis using objectweb asm. Code for that is at > https://github.com/ibuildthecloud/cloudstack-findbadactionevents and there > are instructions to run it there. > > > Diffs > ----- > > api/src/com/cloud/event/ActionEvents.java PRE-CREATION > api/src/org/apache/cloudstack/context/CallContext.java > e3c1bf2a7b97573cdeb4f530d0afe74cb7e3e834 > engine/components-api/src/com/cloud/configuration/ConfigurationManager.java > 6e76b6ffb91c200127589831893d9d79970aafdb > engine/components-api/src/com/cloud/network/rules/FirewallManager.java > fa12cd804a67138740f9d9042709938871dc8629 > server/src/com/cloud/configuration/ConfigurationManagerImpl.java > fb727a1705487416b7069fc2aca5086fd726e700 > server/src/com/cloud/event/ActionEventInterceptor.java > ba7e270af90f7bc191b570a7cc131319f446e2f6 > server/src/com/cloud/event/ActionEventUtils.java > 60f5633fc3c53dac960247308de12b60b492de59 > server/src/com/cloud/network/firewall/FirewallManagerImpl.java > cd83c4e52f85adc9c9d9c7997c28838f2c15b323 > server/src/com/cloud/server/ManagementServerImpl.java > a3efd2129ce082023d79e55872e8134d1b6bd85c > server/src/com/cloud/user/AccountManagerImpl.java > 0602514fcf429b09a62edf65f4b0dc0e87d80b94 > server/src/com/cloud/vm/UserVmManagerImpl.java > c3a718ac55be05f123b062a627c2de042c4321ab > server/test/com/cloud/network/MockFirewallManagerImpl.java > c50459e98737eaf5662bb44c6e9a12fad54b4175 > server/test/com/cloud/vpc/MockConfigurationManagerImpl.java > 3ec146b953726c9480b1e15848a67e4746dd65a6 > > Diff: https://reviews.apache.org/r/14084/diff/ > > > Testing > ------- > > > Thanks, > > Darren Shepherd > >
