cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Will Stevens <williamstev...@gmail.com>
Subject Re: Variable renaming in classes meant for Agents
Date Fri, 20 May 2016 23:30:15 GMT
Unless that PR was not already accounted for in a grandfathered exception.
On May 20, 2016 7:26 PM, "Daan Hoogland" <daan.hoogland@gmail.com> wrote:

> In the mutiny PR I had to change the names that I put final in front so
> that implies line level.
>
> send by tablet, no warranties in existence
> On 21 May 2016 01:19, "Will Stevens" <williamstevens@gmail.com> wrote:
>
> > Does it take offense at a file level or a line of code level? If it is at
> > the file level, then it makes sense because people follow the standard
> > presented in the file and that file is already accounted for in the
> checker
> > threshold.
> > On May 20, 2016 7:06 PM, "Daan Hoogland" <daan.hoogland@gmail.com>
> wrote:
> >
> > Concerning the _s, checkstyle takes offence of those in changed lines of
> > code. So i am wondering how people can get new _s in.
> >
> > send by tablet, no warranties in existence
> > On 21 May 2016 00:55, "Will Stevens" <wstevens@cloudops.com> wrote:
> >
> > > I can confirm we currently have Zero HyperV tests in CI.  Once we have
> > more
> > > people contributing to CI we can try to get better coverage, but right
> > now
> > > I am pretty much just testing on KVM.  I know the accelerite guys are
> > > testing on Xen.  Once I freeze, I will try to do testing of master in
> as
> > > many setups as I can, but I can't do that all the time because I just
> > don't
> > > have the bandwidth (in terms of my time) to be able to do that.
> > >
> > > Our system is far from perfect right now, but I am slowly trying to
> close
> > > that gap...
> > >
> > > *Will STEVENS*
> > > Lead Developer
> > >
> > > *CloudOps* *| *Cloud Solutions Experts
> > > 420 rue Guy *|* Montreal *|* Quebec *|* H3J 1S6
> > > w cloudops.com *|* tw @CloudOps_
> > >
> > > On Fri, May 20, 2016 at 12:49 PM, Tutkowski, Mike <
> > > Mike.Tutkowski@netapp.com
> > > > wrote:
> > >
> > > > Also, does this mean that we have zero Hyper-V integration tests run
> > > > during CI?
> > > > ________________________________________
> > > > From: Tutkowski, Mike <Mike.Tutkowski@netapp.com>
> > > > Sent: Friday, May 20, 2016 10:47 AM
> > > > To: dev@cloudstack.apache.org
> > > > Subject: Re: Variable renaming in classes meant for Agents
> > > >
> > > > Yeah, it has to go into 4.9. :) Unless no one cares about Hyper-V.
> > > > ________________________________________
> > > > From: Rafael Weingärtner <rafaelweingartner@gmail.com>
> > > > Sent: Friday, May 20, 2016 10:42 AM
> > > > To: dev@cloudstack.apache.org
> > > > Subject: Re: Variable renaming in classes meant for Agents
> > > >
> > > > You are right Mike about the “_”. The point is that in some other
> > > language
> > > > the use of “_” makes sense, whereas in Java it does not, at least
not
> > the
> > > > way it has being used in ACS.
> > > >
> > > > We have code conventions, it can be found in [1]. The problem is that
> > it
> > > is
> > > > a bit outdated and I think it could benefit from some others
> tutorials.
> > > For
> > > > instance, a clear and simple tutorial explaining what is a test case
> > and
> > > > showing how to create a proper test case; I am referring to the
> > > > unit/integration test case that we write using Junit and other tools.
> > > >
> > > > Also, we lack some information on how to prepare code to be tested.
> > > > Once we have that kind of standard defined and tutorials written, we
> > > could
> > > > work out ways to educate our community. It is not a problem not to
> know
> > > > those things; we cannot expect everyone to know how to use every
> single
> > > > methodology and technology that is out there. But, we can help people
> > to
> > > > learn, that is the point of a community, it should be a place where
> > > people
> > > > exchange ideas and experience in a way that benefits everyone.
> > > >
> > > > As soon as you open the PR, please let us know, so we can review it
> and
> > > > help you get it merge as soon as possible. I think this is something
> > that
> > > > should go in the 4.9 release.
> > > >
> > > >
> > > > On Fri, May 20, 2016 at 1:27 PM, Tutkowski, Mike <
> > > > Mike.Tutkowski@netapp.com>
> > > > wrote:
> > > >
> > > > > It sounds like most people don't like a preceding "_" for member
> > > > variables
> > > > > and that is fine. Do we have any formal Java coding standards for
> > > > > CloudStack, by the way? I'm not aware of any.
> > > > >
> > > > > The main problem here, though, is that this particular piece of
> code
> > is
> > > > > super fragile, so it would be great to harden it up.
> > > > >
> > > > > I'm going to open a PR and revert the names in those changed
> > "Command"
> > > > > files for 4.9. That will solve the immediate problem.
> > > > > ________________________________________
> > > > > From: Rafael Weingärtner <rafaelweingartner@gmail.com>
> > > > > Sent: Friday, May 20, 2016 9:12 AM
> > > > > To: dev@cloudstack.apache.org
> > > > > Subject: Re: Variable renaming in classes meant for Agents
> > > > >
> > > > > Hi guys,
> > > > > I agree with Daan that if class fields have improper (not
> descriptive
> > > or
> > > > > not suitable) names we should change them. However, I do not find
> the
> > > > > change (on variable names) introduced by PR #816 good. It added an
> > > > > “_”(underline) before variable names; even though Apache CloudStack
> > > has a
> > > > > lot of that currently, I think that is a pattern we should avoid.
> > > > >
> > > > > Your ideas to use annotations to avoid relying on variable names
> are
> > > > great;
> > > > > but, let’s not re-create the well here. There is a research [1]
> that
> > > has
> > > > > been conducted in 2014 that tackled exactly that problem; the
> > proposal
> > > > > presented in [1] decoupled client and server sides from variable
> name
> > > by
> > > > > using semantic annotations. The concept, the formalization and the
> > > > > experiments are all presented in paper [1]. The serialization and
> > > > > deserialization core of the proposal presented in [1] can be found
> in
> > > > [2].
> > > > >
> > > > > The idea of decoupling our web APIs from variable names is great,
> but
> > > it
> > > > is
> > > > > something that will require some effort to be fully and properly
> > > > > implemented. If you wish to push that forward count on me.
> > > > >
> > > > > [1]
> > http://ieeexplore.ieee.org/xpls/abs_all.jsp?arnumber=6928953&tag=1
> > > > > [2] https://github.com/ivansalvadori/gsonld
> > > > >
> > > > >
> > > > > On Fri, May 20, 2016 at 3:30 AM, Daan Hoogland <
> > > daan.hoogland@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Guys, we should rename fields that have improper names. I do
not
> > > agreee
> > > > > > with the statement at all. Your two solutions to the
> serialisation
> > > > hazard
> > > > > > are both acceptable to me. leaving a non compliant or non
> > explanatory
> > > > > name
> > > > > > in because it slipped through the nets at some points does not
> seem
> > > > > > acceptable to me. We must improve are code.
> > > > > >
> > > > > > On Fri, May 20, 2016 at 6:53 AM, Tutkowski, Mike <
> > > > > > Mike.Tutkowski@netapp.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Thanks for sending out this e-mail, Anshul.
> > > > > > >
> > > > > > > This is a bit of a strange situation because we need to
make
> sure
> > > > > people
> > > > > > > are either aware of the fact that properties in Command
classes
> > are
> > > > > > > serialized (and not change existing variable names) or
come up
> > > with a
> > > > > > less
> > > > > > > fragile way of choosing property names when sending data
> (perhaps
> > > > using
> > > > > > > annotations).
> > > > > > >
> > > > > > > At the very least, we should have comments in these classes
> > > > indicating
> > > > > > the
> > > > > > > dangers of changing property names. It might also be beneficial
> > to
> > > > have
> > > > > > > unit tests in place that expect certain variable names
and
> assert
> > > if
> > > > > they
> > > > > > > are not as expected.
> > > > > > >
> > > > > > > In the meanwhile, I plan to change the variable names back
that
> > > were
> > > > > > > changed in PR #816.
> > > > > > >
> > > > > > > Additional thoughts on how this should be addressed long
term?
> > > > > > >
> > > > > > > Thanks!
> > > > > > > Mike
> > > > > > > ________________________________________
> > > > > > > From: Anshul Gangwar <anshul.gangwar@accelerite.com>
> > > > > > > Sent: Thursday, May 19, 2016 10:47 PM
> > > > > > > To: dev@cloudstack.apache.org
> > > > > > > Subject: Variable renaming in classes meant for Agents
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > We should not allow renaming of variables in classes which
ends
> > > with
> > > > > > > Command and TO. As these objects are meant to be consumed
by
> > > Agents.
> > > > > > >
> > > > > > > Agents may not be written in java so relying on these variable
> > > names
> > > > to
> > > > > > > get the info. One such example is Hyper-V agent.
> > > > > > >
> > > > > > > Hyper-V support is currently broken as there are some variables
> > > > renamed
> > > > > > in
> > > > > > > PR https://github.com/apache/cloudstack/pull/816.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Anshul
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > DISCLAIMER
> > > > > > > ==========
> > > > > > > This e-mail may contain privileged and confidential information
> > > which
> > > > > is
> > > > > > > the property of Accelerite, a Persistent Systems business.
It
> is
> > > > > intended
> > > > > > > only for the use of the individual or entity to which it
is
> > > > addressed.
> > > > > If
> > > > > > > you are not the intended recipient, you are not authorized
to
> > read,
> > > > > > retain,
> > > > > > > copy, print, distribute or use this message. If you have
> received
> > > > this
> > > > > > > communication in error, please notify the sender and delete
all
> > > > copies
> > > > > of
> > > > > > > this message. Accelerite, a Persistent Systems business
does
> not
> > > > accept
> > > > > > any
> > > > > > > liability for virus infected mails.
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Daan
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Rafael Weingärtner
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Rafael Weingärtner
> > > >
> > >
> >
>

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