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:08:17 GMT
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
Mime
View raw message