You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Syed Mushtaq <sy...@gmail.com> on 2016/05/19 18:37:10 UTC
Refactoring CitrixResourceBase
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
Re: Refactoring CitrixResourceBase
Posted by Will Stevens <ws...@cloudops.com>.
cool. :)
*Will STEVENS*
Lead Developer
*CloudOps* *| *Cloud Solutions Experts
420 rue Guy *|* Montreal *|* Quebec *|* H3J 1S6
w cloudops.com *|* tw @CloudOps_
On Wed, May 25, 2016 at 9:45 PM, Syed Mushtaq <sy...@gmail.com>
wrote:
> Nope. I think they should all be XenServer (they were Citrix earlier) I
> plan to clean that up too.
>
> -Syed
>
> On Wed, May 25, 2016 at 9:43 PM, Will Stevens <wi...@gmail.com>
> wrote:
>
> > Is there a reason some are prefixed with 'Xs' and others are prefixed
> with
> > 'XenServer'?
> > On May 25, 2016 9:41 PM, "Will Stevens" <wi...@gmail.com>
> wrote:
> >
> > > Ya. I didn't know either and made the same mistake. :)
> > > On May 25, 2016 9:40 PM, "Syed Mushtaq" <sy...@gmail.com>
> wrote:
> > >
> > >> Aah I did not know that. Thanks Will .. here is the link
> > >> http://i.imgur.com/5B55XMB.png
> > >>
> > >> On Wed, May 25, 2016 at 6:37 PM, Will Stevens <
> williamstevens@gmail.com
> > >
> > >> wrote:
> > >>
> > >> > 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" <sa...@cloudops.com> wrote:
> > >> >
> > >> > > Forgot to attach screenshot
> > >> > >
> > >> > > On Wed, May 25, 2016 at 4:09 PM, Syed Ahmed <sa...@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
> > >> > >>>
> > >> > >>
> > >> > >>
> > >> > >
> > >> >
> > >>
> > >
> >
>
Re: Refactoring CitrixResourceBase
Posted by Syed Mushtaq <sy...@gmail.com>.
Nope. I think they should all be XenServer (they were Citrix earlier) I
plan to clean that up too.
-Syed
On Wed, May 25, 2016 at 9:43 PM, Will Stevens <wi...@gmail.com>
wrote:
> Is there a reason some are prefixed with 'Xs' and others are prefixed with
> 'XenServer'?
> On May 25, 2016 9:41 PM, "Will Stevens" <wi...@gmail.com> wrote:
>
> > Ya. I didn't know either and made the same mistake. :)
> > On May 25, 2016 9:40 PM, "Syed Mushtaq" <sy...@gmail.com> wrote:
> >
> >> Aah I did not know that. Thanks Will .. here is the link
> >> http://i.imgur.com/5B55XMB.png
> >>
> >> On Wed, May 25, 2016 at 6:37 PM, Will Stevens <williamstevens@gmail.com
> >
> >> wrote:
> >>
> >> > 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" <sa...@cloudops.com> wrote:
> >> >
> >> > > Forgot to attach screenshot
> >> > >
> >> > > On Wed, May 25, 2016 at 4:09 PM, Syed Ahmed <sa...@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
> >> > >>>
> >> > >>
> >> > >>
> >> > >
> >> >
> >>
> >
>
Re: Refactoring CitrixResourceBase
Posted by Will Stevens <wi...@gmail.com>.
Is there a reason some are prefixed with 'Xs' and others are prefixed with
'XenServer'?
On May 25, 2016 9:41 PM, "Will Stevens" <wi...@gmail.com> wrote:
> Ya. I didn't know either and made the same mistake. :)
> On May 25, 2016 9:40 PM, "Syed Mushtaq" <sy...@gmail.com> wrote:
>
>> Aah I did not know that. Thanks Will .. here is the link
>> http://i.imgur.com/5B55XMB.png
>>
>> On Wed, May 25, 2016 at 6:37 PM, Will Stevens <wi...@gmail.com>
>> wrote:
>>
>> > 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" <sa...@cloudops.com> wrote:
>> >
>> > > Forgot to attach screenshot
>> > >
>> > > On Wed, May 25, 2016 at 4:09 PM, Syed Ahmed <sa...@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
>> > >>>
>> > >>
>> > >>
>> > >
>> >
>>
>
Re: Refactoring CitrixResourceBase
Posted by Will Stevens <wi...@gmail.com>.
Ya. I didn't know either and made the same mistake. :)
On May 25, 2016 9:40 PM, "Syed Mushtaq" <sy...@gmail.com> wrote:
> Aah I did not know that. Thanks Will .. here is the link
> http://i.imgur.com/5B55XMB.png
>
> On Wed, May 25, 2016 at 6:37 PM, Will Stevens <wi...@gmail.com>
> wrote:
>
> > 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" <sa...@cloudops.com> wrote:
> >
> > > Forgot to attach screenshot
> > >
> > > On Wed, May 25, 2016 at 4:09 PM, Syed Ahmed <sa...@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
> > >>>
> > >>
> > >>
> > >
> >
>
Re: Refactoring CitrixResourceBase
Posted by Syed Mushtaq <sy...@gmail.com>.
Aah I did not know that. Thanks Will .. here is the link
http://i.imgur.com/5B55XMB.png
On Wed, May 25, 2016 at 6:37 PM, Will Stevens <wi...@gmail.com>
wrote:
> 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" <sa...@cloudops.com> wrote:
>
> > Forgot to attach screenshot
> >
> > On Wed, May 25, 2016 at 4:09 PM, Syed Ahmed <sa...@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 <tm...@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
> >>>
> >>
> >>
> >
>
Re: Refactoring CitrixResourceBase
Posted by Will Stevens <wi...@gmail.com>.
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" <sa...@cloudops.com> wrote:
> Forgot to attach screenshot
>
> On Wed, May 25, 2016 at 4:09 PM, Syed Ahmed <sa...@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 <sy...@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 <tm...@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
>>>
>>
>>
>
Re: Refactoring CitrixResourceBase
Posted by Syed Ahmed <sa...@cloudops.com>.
Forgot to attach screenshot
On Wed, May 25, 2016 at 4:09 PM, Syed Ahmed <sa...@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 <sy...@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 <sy...@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 <tm...@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
>>
>
>
Re: Refactoring CitrixResourceBase
Posted by Rafael Weingärtner <ra...@gmail.com>.
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 <sy...@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 <
> rafaelweingartner@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 <sa...@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
> > > >
> > >
> >
> >
> >
> > --
> > Rafael Weingärtner
> >
>
--
Rafael Weingärtner
Re: Refactoring CitrixResourceBase
Posted by Syed Mushtaq <sy...@gmail.com>.
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 <
rafaelweingartner@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 <sa...@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 <tm...@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
> > >
> >
>
>
>
> --
> Rafael Weingärtner
>
Re: Refactoring CitrixResourceBase
Posted by Rafael Weingärtner <ra...@gmail.com>.
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 <sa...@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 <sy...@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 <tm...@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
> >
>
--
Rafael Weingärtner
Re: Refactoring CitrixResourceBase
Posted by Syed Ahmed <sa...@cloudops.com>.
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 <sy...@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 <sy...@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 <tm...@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
>
Re: Refactoring CitrixResourceBase
Posted by Rafael Weingärtner <ra...@gmail.com>.
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 <sy...@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 <sy...@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 <tm...@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
Re: Refactoring CitrixResourceBase
Posted by Syed Mushtaq <sy...@gmail.com>.
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 <sy...@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 <da...@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 <tm...@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
>>
>
>
Re: Refactoring CitrixResourceBase
Posted by Syed Mushtaq <sy...@gmail.com>.
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 <da...@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 <tm...@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
>
Re: Refactoring CitrixResourceBase
Posted by Rafael Weingärtner <ra...@gmail.com>.
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 <da...@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 <tm...@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 <sy...@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
Re: Refactoring CitrixResourceBase
Posted by Daan Hoogland <da...@gmail.com>.
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 <tm...@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 <sy...@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
Re: Refactoring CitrixResourceBase
Posted by Tim Mackey <tm...@gmail.com>.
+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 <sy...@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
>