Got it, maybe when you open the PR we can work together to use spring framework to manage those beans.
On Thu, May 26, 2016 at 2:05 PM, Syed Mushtaq <syed1.mush...@gmail.com> wrote: > Yes Rafae. I saw that XenServer classes are not plugged into Spring. I am > instantiating the objects manually in the configure() method of the base > class and using them. > > On Thu, May 26, 2016 at 12:16 PM, Rafael Weingärtner < > rafaelweingart...@gmail.com> wrote: > > > Indeed, this is a non-trivial job, and it will take a pretty good amount > of > > time and effort. > > > > Sorry, I did not understand what you meant with "reference" file. Is that > > the base file that you are using to create a hierarchy? > > > > The solution you are creating looks promising, I am just wondering; you > > said that you are creating singletons, you probably are thinking about > > Spring singletons, right? There is only one problem, the > > “CitrixResourceBase” and its subclasses were not being wired using > > Spring-framework. Have you noticed that? > > > > To add those new classes (singletons) you are creating and wire them > > properly into the Spring life cycle may require some work, if you ran > into > > a wall, please let us know, so we can help you. > > > > On Wed, May 25, 2016 at 5: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 > > > > > > > > > > > > > > > -- > > Rafael Weingärtner > > > -- Rafael Weingärtner
