Attachments don't work. You will have to host it publicly and then post a link to it. On May 25, 2016 4:28 PM, "Syed Ahmed" <sah...@cloudops.com> wrote:
> 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 >>> >> >> >
