cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rohit Yadav <rohit.ya...@shapeblue.com>
Subject Re: [GitHub] rafaelweingartner commented on a change in pull request #2416: CLOUDSTACK-10244: KVM online storage migration fails
Date Sun, 21 Jan 2018 10:19:05 GMT
Thanks for the discussion, I'll prefer keeping the current code as well.

Merging this based on code reviews and tests.



- Rohit

<https://cloudstack.apache.org>



________________________________
From: Rafael Weingärtner <rafaelweingartner@gmail.com>
Sent: Saturday, January 20, 2018 11:29:38 PM
To: dev
Subject: Re: [GitHub] rafaelweingartner commented on a change in pull request #2416: CLOUDSTACK-10244:
KVM online storage migration fails

>
> So, your official position is that declaring a variable as constant is
> typically pointless because some future programmer can always change the
> variable to not be constant


That is it! I do that a lot.. specially if I cannot find a reason to
declare a variable final. That is why quality code documentation is very
important.

On Sat, Jan 20, 2018 at 3:53 PM, Tutkowski, Mike <Mike.Tutkowski@netapp.com>
wrote:

> So, your official position is that declaring a variable as constant is
> typically pointless because some future programmer can always change the
> variable to not be constant. :)
>
> In this case, to appease both parties (you and Wido), I’ll leave it final,
> but add a comment to explain why it’s final.
>
> It should end up being a moot point in 4.12 as I plan to change the
> replaceStorage method to create a shallow copy of the passed-in map and
> mutate it as need be. Then, we won’t even need this Boolean.
>
> > On Jan 20, 2018, at 9:58 AM, Rafael Weingärtner <
> rafaelweingartner@gmail.com> wrote:
> >
> > No setters help to make an object immutable, but in Java we have
> > reflection, and the only way to really avoid changing a property is using
> > the final modifier. However, even when using final, it is possible to do
> so
> > by manipulating the byte code of the class that describes the object and
> is
> > loaded to the JVM. This is what PowerMock does to deal with static and
> > final method/variable mocking.
> >
> > On Sat, Jan 20, 2018 at 2:40 PM, Tutkowski, Mike <
> Mike.Tutkowski@netapp.com>
> > wrote:
> >
> >> “For instance, when we have to design/create a library and we want to
> make
> >> sure an object is immutable.”
> >>
> >> According to your argument, though, you don’t need final here either:
> just
> >> make sure to never provide setters or change those values (and
> document).
> >>
> >>> On Jan 20, 2018, at 9:37 AM, Tutkowski, Mike <
> Mike.Tutkowski@netapp.com>
> >> wrote:
> >>>
> >>> Personally, I’m OK with it either way here. If @wido reads what you
> >> wrote and asks me to change it back to the way I wrote it initially, I’m
> >> happy to do so.
> >>>
> >>> I believe he sees the explicitly declared constant here not only as a
> >> protection against ourselves, but to prevent a future programmer from
> >> easily making a mistake. At least now, if this future programmer changes
> >> the value, the compile will fail and he/she will be forced to think
> through
> >> the repercussions of doing so.
> >>>
> >>>> On Jan 20, 2018, at 9:30 AM, Rafael Weingärtner <
> >> rafaelweingartner@gmail.com> wrote:
> >>>>
> >>>> Never is a strong word. In any case, let it be if you believe it is
> >> going
> >>>> to provide  benefits…
> >>>>
> >>>> I believe `final` modifier should be used in certain situation when
it
> >> is
> >>>> truly required. For instance, when we have to design/create a library
> >> and
> >>>> we want to make sure an object is immutable. Then, we configure all
of
> >> its
> >>>> variables as final. Now, declaring a variable final in the middle of
a
> >>>> method to protect against ourselves…. It does not seem to bring much
> >>>> benefit against future mistaken/accidental changes. It is always
> >> possible
> >>>> for the hypothetical future programmer to remove the final and change
> >> the
> >>>> variable. Perhaps a better documentation for the method explaining
> this
> >>>> situations could bring more benefits. A single variable declared as
> >> final
> >>>> does not give a hint for people on why it was made final.
> >>>>
> >>>> On Sat, Jan 20, 2018 at 2:08 PM, Tutkowski, Mike <
> >> Mike.Tutkowski@netapp.com>
> >>>> wrote:
> >>>>
> >>>>> Perhaps your argument against the final keyword is that it should
> >> never be
> >>>>> used? If so, you and @wido can debate that and let me know which
way
> >> you’d
> >>>>> like this bit of code to end up.
> >>>>>
> >>>>>> On Jan 20, 2018, at 9:05 AM, Tutkowski, Mike <
> >> Mike.Tutkowski@netapp.com>
> >>>>> wrote:
> >>>>>>
> >>>>>> It’s pretty much the reason why the final variable exists
in Java:
> to
> >>>>> make a variable a constant so no one accidentally changes it. @wido
> >> just
> >>>>> wanted the code to protect against the variable being
> changed...that’s
> >> all.
> >>>>>>
> >>>>>>> On Jan 20, 2018, at 8:25 AM, Rafael Weingärtner <
> >>>>> rafaelweingartner@gmail.com> wrote:
> >>>>>>>
> >>>>>>> Changed by a future programmer that may develop some changes
in
> your
> >>>>> code?
> >>>>>>> Because, if you do not code any other assignment to that
variable,
> it
> >>>>> will
> >>>>>>> never happen.
> >>>>>>>
> >>>>>>> On Sat, Jan 20, 2018 at 1:16 PM, Tutkowski, Mike <
> >>>>> Mike.Tutkowski@netapp.com>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> @wido just wanted to make sure the Boolean variable
didn’t
> >> accidentally
> >>>>>>>> get changed later since it’s critical it keep that
value.
> >>>>>>>>
> >>>>>>>>> On Jan 20, 2018, at 8:14 AM, Tutkowski, Mike <
> >>>>> Mike.Tutkowski@netapp.com>
> >>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>> I made it final per @wido’s comment (in #2415).
How’s about you
> >> guys
> >>>>>>>> hash it out and get back to me. :)
> >>>>>>>>>
> >>>>>>>>>
rohit.yadav@shapeblue.com 
www.shapeblue.com
53 Chandos Place, Covent Garden, London  WC2N 4HSUK
@shapeblue
  
 

> On Jan 20, 2018, at 8:06 AM, GitBox <git@apache.org> wrote:
> >>>>>>>>>>
> >>>>>>>>>> rafaelweingartner commented on a change in pull
request #2416:
> >>>>>>>> CLOUDSTACK-10244: KVM online storage migration fails
> >>>>>>>>>> URL: https://github.com/apache/cloudstack/pull/2416#
> >>>>>>>> discussion_r162784630
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> ##########
> >>>>>>>>>> File path: plugins/hypervisors/kvm/src/
> com/cloud/hypervisor/kvm/
> >>>>>>>> resource/wrapper/LibvirtMigrateCommandWrapper.java
> >>>>>>>>>> ##########
> >>>>>>>>>> @@ -132,8 +132,9 @@ Use VIR_DOMAIN_XML_SECURE
(value = 1) prior
> to
> >>>>>>>> v1.0.0.
> >>>>>>>>>>       vmsnapshots = libvirtComputingResource.
> >>>>>>>> cleanVMSnapshotMetadata(dm);
> >>>>>>>>>>
> >>>>>>>>>>       Map<String, MigrateCommand.MigrateDiskInfo>
> >>>>>>>> mapMigrateStorage = command.getMigrateStorage();
> >>>>>>>>>> +            final boolean migrateStorage =
MapUtils.isNotEmpty(
> >>>>>>>> mapMigrateStorage);
> >>>>>>>>>>
> >>>>>>>>>> Review comment:
> >>>>>>>>>> This variable does not need to be final. It
does not bring any
> >>>>>>>> benefits here.
> >>>>>>>>>>
> >>>>>>>>>> You needed to assign the result of the first
> `MapUtils.isNotEmpty(
> >>>>> mapMigrateStorage)`
> >>>>>>>> because the method `replaceStorage` might change the
status of
> >>>>>>>> `mapMigrateStorage`, right?
> >>>>>>>>>>
> >>>>>>>>>> ------------------------------------------------------------
> ----
> >>>>>>>>>> This is an automated message from the Apache
Git Service.
> >>>>>>>>>> To respond to the message, please log on GitHub
and use the
> >>>>>>>>>> URL above to go to the specific comment.
> >>>>>>>>>>
> >>>>>>>>>> For queries about this service, please contact
Infrastructure
> at:
> >>>>>>>>>> users@infra.apache.org
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> With regards,
> >>>>>>>>>> Apache Git Services
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> --
> >>>>>>> Rafael Weingärtner
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> Rafael Weingärtner
> >>
> >
> >
> >
> > --
> > Rafael Weingärtner
>



--
Rafael Weingärtner

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