Thanks Rafael, Here is how I am doing. You can see the tree in the screenshot attached. I have started with storage and extracted the storage commands out of CitrixResourceBase (I've renamed it to XenServerResourceBase). This is the file for refrence. Let me know what you think.
https://github.com/syed/cloudstack/blob/ca2d1469f991727515d9b71a790411d36eb60fdc/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/storage/XenServerStorageResource.java It is a non-trivial refactor so may take some time but I think the end result would be very good for everyone. On Wed, May 25, 2016 at 11:22 AM, Rafael Weingärtner < rafaelweingart...@gmail.com> wrote: > Hi Syed, > That is a great job. > I would only suggest breaking the commons a little bit more between > “monitoring” and “common”. On the monitoring side, we could have host > monitoring, VMs monitoring, VMs' status checks (running, stopped, and > others) and maybe other tasks that aim to monitor/check a resource > state/information. > > About the singletons you extracted, I do not see a need for an interface > right now (maybe I am overlooking the need); in the future if the need for > interfaces appears, we can create some interfaces and use them into the > singletons you created. > > > On Wed, May 25, 2016 at 12:10 PM, Syed Mushtaq <syed1.mush...@gmail.com> > wrote: > > > Hey Guys, > > > > To give you an update, I've identified and categorized the functions in > > CitrixResourceBase into 4 categories > > > > 1) common: These deal with the host as a whole (example getHostInfo, > > callPlugin, connection pool etc) > > 2) compute: Dealing with operations on VMs (start,stop, reboot, update > etc) > > 3) storage: Dealing with storage operations (creating VDI, attaching to > VM, > > SR operations) > > 4) network: OVS / PIF / VIF operations > > > > I've created singleton classes for each and slowly moving the > functionality > > there. These singletons don't have a base class nor do they implement an > > interface. I don't know what the best practice is. One question that I > have > > is, should I create an interface for them or use some existing interface? > > > > Thanks, > > -Syed > > > > > > On Fri, May 20, 2016 at 9:58 AM, Syed Mushtaq <syed1.mush...@gmail.com> > > wrote: > > > > > Thanks guys for the Ideas. I will open a JIRA ticket and start working > on > > > it. > > > > > > -Syed > > > > > > > > > On Thu, May 19, 2016 at 7:57 PM, Rafael Weingärtner < > > > rafaelweingart...@gmail.com> wrote: > > > > > >> Hi Syed, > > >> That is a great idea; however, it is a very hard task. > > >> The idea of Tim is great; actually, we already have some sort of > > hierarchy > > >> that is used in “CitrixResourceBase.java”. > > >> I would suggest you first removing the unused code, unused variable, > and > > >> duplicate methods; that would be one PR. You can use a tool called > > >> UCdetector to find unused code. To find duplicated code you can use > PMD. > > >> > > >> One very good example of code duplicity are the methods called > > >> > > >> > > > “com.cloud.hypervisor.xenserver.resource.CitrixResourceBase.callHostPluginAsync(…)”. > > >> > > >> After you have cleaned the class, I suggest you analyzing where each > > >> remaining method is used and then look for the proper place to put > them. > > >> It might be a good idea on separating between singletons that are > > >> responsible for well-defined tasks such as managing storage, > networking, > > >> VMs creating and deletion, VMs monitoring and others. > > >> > > >> If you need any help, please do not hesitate on asking for our help. > > >> > > >> > > >> On Thu, May 19, 2016 at 4:50 PM, Daan Hoogland < > daan.hoogl...@gmail.com > > > > > >> wrote: > > >> > > >> > Syed, > > >> > > > >> > gogogo. actually it has shrunk to 5k lines since 2012 ;) > > >> > > > >> > I like your initiative and initial direction. A lot of small steps > to > > >> > improve the blob have been taken and I would sugest to keep going in > > >> small > > >> > steps. > > >> > > > >> > On Thu, May 19, 2016 at 9:44 PM, Tim Mackey <tmac...@gmail.com> > > wrote: > > >> > > > >> > > +1 > > >> > > > > >> > > When I went through this last time, not only was it hard to > > understand > > >> > the > > >> > > flows, but the XenServer version management was a pain. Would > > suggest > > >> > > creating a base class which always works (i.e. is independent of > > >> > XenServer > > >> > > version) for core functions. Then add in that which exists for a > > >> specific > > >> > > version. Should help greatly with testing IMO. > > >> > > > > >> > > -tim > > >> > > > > >> > > On Thu, May 19, 2016 at 2:37 PM, Syed Mushtaq < > > >> syed1.mush...@gmail.com> > > >> > > wrote: > > >> > > > > >> > > > Hi All, > > >> > > > > > >> > > > I would like to refactor CitrixResourceBase class which is > > >> responsible > > >> > > for > > >> > > > communicating with Xenserver. It has grown too long (>5K lines) > > and > > >> has > > >> > > > absolutely no testing. > > >> > > > > > >> > > > In my first pass I want to separate out the functionality buy > the > > >> > > subsystem > > >> > > > it targets (compute, storage, network etc) and will go on from > > >> there. > > >> > > What > > >> > > > do you think? Is anyone working on this currently? > > >> > > > > > >> > > > Thanks, > > >> > > > -Syed > > >> > > > > > >> > > > > >> > > > >> > > > >> > > > >> > -- > > >> > Daan > > >> > > > >> > > >> > > >> > > >> -- > > >> Rafael Weingärtner > > >> > > > > > > > > > > > > -- > Rafael Weingärtner >
