cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tutkowski, Mike" <Mike.Tutkow...@netapp.com>
Subject Re: [GitHub] rafaelweingartner commented on a change in pull request #2416: CLOUDSTACK-10244: KVM online storage migration fails
Date Sat, 20 Jan 2018 16:36:48 GMT
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. :)
>>>>>> 
>>>>>>> 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
Mime
View raw message