cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Will Stevens <williamstev...@gmail.com>
Subject Re: Refactoring CitrixResourceBase
Date Wed, 25 May 2016 22:37:15 GMT
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" <sahmed@cloudops.com> wrote:

> Forgot to attach screenshot
>
> On Wed, May 25, 2016 at 4:09 PM, Syed Ahmed <sahmed@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 <
>> rafaelweingartner@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.mushtaq@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.mushtaq@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 <
>>> > > rafaelweingartner@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.hoogland@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 <tmackey@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.mushtaq@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
>>>
>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message