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 <rafaelweingart...@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.tutkow...@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 < > rafaelweingart...@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.tutkow...@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.tutkow...@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 < > >> rafaelweingart...@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.tutkow...@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.tutkow...@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 < > >>>>> rafaelweingart...@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.tutkow...@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.tutkow...@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.ya...@shapeblue.com www.shapeblue.com 53 Chandos Place, Covent Garden, London WC2N 4HSUK @shapeblue > On Jan 20, 2018, at 8:06 AM, GitBox <g...@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: > >>>>>>>>>> us...@infra.apache.org > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> With regards, > >>>>>>>>>> Apache Git Services > >>>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> -- > >>>>>>> Rafael Weingärtner > >>>>> > >>>> > >>>> > >>>> > >>>> -- > >>>> Rafael Weingärtner > >> > > > > > > > > -- > > Rafael Weingärtner > -- Rafael Weingärtner
