From: Alex Huang Sent: Tuesday, February 05, 2013 8:57 PM To: 'Joe San' Subject: RE: [Refactor AccountManagerImpl][DISCUSS]
Joe, I was commenting on the fact that AccountManagerImpl is not done in this style, which causes all the different dependencies including dao stuff. To refactor AccountManagerImpl, I think the following needs to be done. You may know this well already but just for completeness, I include everything I can think so. - Make sure all states for account/users are properly enumerated. It looks like it should be enumerated on Account.java - Add dependency on something like google guava's event bus - Move the different parts of AccountManagerImpl into the respective managers. i.e. vm cleanup to userVmManagerImpl.cleanupOnAccountDeletion, volumes back to userVmManagerImpl.cleanupOnAccountDeletion. - Add the appropriate annotation from event bus to the methods. - Register with the event bus in the configure() method of the managers. - In AccontManagerImpl, implement a processing loop that just manages the state of the account and publish the new state. For example on account deletion: o Set the state to delete to prevent the account from logging in. o Signal to the event bus the new state along with the Account object. o If any of the listeners throws an error, mark the account as cleanup required and signal error to the administrator. o If the administrator figures out what happened, they should correct the problem and then delete the account again. o If the account cleanup is successful, remove any cleanup required attribute and any unique keys in the account row and then mark the row as removed. AccountManager as it is today should only control account and users so it having access to account dao and user dao is fine but it shouldn't access other daos. After this, you should be able to write a unit tests that does the following: - inject a mocked listener, mocked daos, and mocked alertmanager - run account actions (create/delete/update etc) - invoke error inside the mocked listener and then check if the account has the appropriate states and the appropriate alerts have fired. - run account actions again and this time have the mocked listener succeed and see that the mocked dao was invoked to remove the account at the end. --Alex From: Joe San [mailto:[email protected]] Sent: Monday, February 04, 2013 2:04 PM To: Alex Huang Subject: Re: [Refactor AccountManagerImpl][DISCUSS] Alex, Thanks for the email. I understand the listener model of doing things. But the AccountManager class has methods other than delete as well. As I could see from the source code that the AccountManager#deleteAccount has most of the dao calls in the cleanupAccount, I guess we need to refactor that. I could try that. >From your email below: - The thread actually will go to all of the listeners and processes delete so code to handle vms, volumes, rules, ip addresses etc all reside in their own managers. Could you let me know which listeners are you talking about? Regards, Jothi On Sat, Feb 2, 2013 at 3:25 PM, Alex Huang <[email protected]<mailto:[email protected]>> wrote: This is a very good topic. Here's my $.02 on this. The reason why AccountMgr has so many dependencies is because of the following: "Oh deleting an account? I have to delete all the vms, I have to delete the volumes, I have to remove all firewall rules, I have to release ip address, etc" So the developers naturally add it to AccountMgr since they're all part of an account deletion. Imagine that for all of accountmanager's functions. Then that tightly couples the internals of CloudStack. What it really should do is follow the pattern set in AgentManager. AgentManager says I take care of agent connections but if you're interested in doing processing due to agent connection loss/connect, then register a listener with me and I will publish the agent connection events. That way all other managers are decoupled from AgentManager. Now, this is different from a pure publisher/subscriber model because in this we don't want the processing to be completely independent of each other since the account manager does want to know if some resources have not been cleaned up. We just want the code to be decoupled. In this model, it would work like this. On delete: - AccountManager sets the state of the account to 'Deleted'. This causes the account to not be able to login. - AccountManager fires a DELETE event. - The thread actually will go to all of the listeners and processes delete so code to handle vms, volumes, rules, ip addresses etc all reside in their own managers. - This event may cause ripple effects throughout the Managers as each Manager should have their own listener structure. - Once all of that is done, then AccountManager finally removes the row from the table. On error: - AccountManager appropriately notifies the caller. - The caller then fixes the problem and calls delete again. - Account Manager then again goes through the procedure above. All of CloudStack's managers should behave in this pattern. I would encourage all of the developers to study this and contact me with any questions. --Alex > -----Original Message----- > From: Joe San [mailto:[email protected]<mailto:[email protected]>] > Sent: Saturday, February 02, 2013 5:57 AM > To: > [email protected]<mailto:[email protected]> > Subject: [Refactor AccountManagerImpl][DISCUSS] > > Cloudstack dev team, > > I would like to propose to refactor the AccountManagerImpl.java. The idea > would be to make the implementation without any instance fields. At > present > there are a lot of Dao's being injected. This brings a lot of cross > references to other dao's as well. Ideally AccountManagerImpl only needs > the AccountDAO and should only contain methods pertaining to Account > related activities. Any other DAO calls like NetworkDAO should be handled > through the NetworkManagerImpl (needs to be created if one does not > exist). > > I would love to see AccountManagerImpl free of any injections. > > Regards, > Joe
