You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2018/01/20 15:05:52 UTC

[GitHub] rafaelweingartner commented on a change in pull request #2416: CLOUDSTACK-10244: KVM online storage migration fails

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

Re: [GitHub] rafaelweingartner commented on a change in pull request #2416: CLOUDSTACK-10244: KVM online storage migration fails

Posted by Rohit Yadav <ro...@shapeblue.com>.

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 <ra...@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 <Mi...@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 <gi...@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