Forgot to attach screenshot On Wed, May 25, 2016 at 4:09 PM, Syed Ahmed <sah...@cloudops.com> wrote:
> 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 >> > >
