> On June 13, 2013, 6:01 p.m., John Burwell wrote:
> >
>
> Wei Zhou wrote:
> John,
>
> The validation of input fields is in CreateDiskOfferingCmd.java and
> CreateServiceOfferingCmd.java, like:
> public long getBytesReadRate() {
> return (bytesReadRate == null) || (bytesReadRate < 0) ? 0 :
> bytesReadRate;
> }
> It can avoid setting a variaty with "long" type to "null" in other java
> files, for instance VolumeTO.java and DiskProfile.java.
>
> I think it is better to set the default value to 0 which means no
> limitation.
> What do you think if the default value is null? Maybe it also means no
> limitation, right?
>
> -Wei
>
> John Burwell wrote:
> null is the Java idiom for representing the lack of a optional value
> across all types. Using zero for this purpose feels like a magic value. It
> also raises the question of a client's intent when zero is passed -- did they
> make a mistake or did they intend not specify a value? To my way of
> thinking, any non-null value for the rates should be greater than zero and
> less than a maximum. Values that fall out of this range cause an exception
> ...
>
> Wei Zhou wrote:
> The default value means, if user does not specify or specify to an
> invalid value, the variaty will be set to default.
> If add null value, the processing for null is same to 0. There is no
> special processing for null value, as both null and 0 mean no limitation in
> Java side.
> maximum is not necessary, I think, unless there is a cap in hypervisor. I
> do not see the cap in libvirt.
If the value's content wasn't being checked as a business rule in
LibVirtComputingResource, I could see it as a default value. However, there
are parts of the code that need to check for presence, and null is the most
natural expression of that case in Java. In the context of this patch, it
seems relatively minor. However, across a large code base, using null
consistently across all types to represent the lack of an optional value makes
the code base much easier to comprehend and manage,
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11782/#review21865
-----------------------------------------------------------
On June 12, 2013, 9:13 a.m., Wei Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11782/
> -----------------------------------------------------------
>
> (Updated June 12, 2013, 9:13 a.m.)
>
>
> Review request for cloudstack, Wido den Hollander and John Burwell.
>
>
> Description
> -------
>
> The patch for VM Disk I/O throttling based on commit
> 3f3c6aa35f64c4129c203d54840524e6aa2c4621
>
>
> This addresses bug CLOUDSTACK-1301.
>
>
> Diffs
> -----
>
> api/src/com/cloud/agent/api/to/VolumeTO.java 4cbe82b
> api/src/com/cloud/offering/DiskOffering.java dd77c70
> api/src/com/cloud/vm/DiskProfile.java e3a3386
> api/src/org/apache/cloudstack/api/ApiConstants.java ab1402c
>
> api/src/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java
> aa11599
>
> api/src/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java
> 4c54a4e
> api/src/org/apache/cloudstack/api/response/DiskOfferingResponse.java
> 377e66e
> api/src/org/apache/cloudstack/api/response/ServiceOfferingResponse.java
> 31533f8
> api/src/org/apache/cloudstack/api/response/VolumeResponse.java 21d7d1a
> client/WEB-INF/classes/resources/messages.properties 2b17359
> core/src/com/cloud/agent/api/AttachVolumeCommand.java 302b8f8
> engine/schema/src/com/cloud/storage/DiskOfferingVO.java 909d7fe
>
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
> bab53bc
>
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java
> b8645e1
>
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
> 9cddb2e
> server/src/com/cloud/api/query/dao/DiskOfferingJoinDaoImpl.java 283181f
> server/src/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java 56e4d0a
> server/src/com/cloud/api/query/dao/VolumeJoinDaoImpl.java e27e2d9
> server/src/com/cloud/api/query/vo/DiskOfferingJoinVO.java 6d3cdcb
> server/src/com/cloud/api/query/vo/ServiceOfferingJoinVO.java e87a101
> server/src/com/cloud/api/query/vo/VolumeJoinVO.java 6ef8c91
> server/src/com/cloud/configuration/Config.java 5ee0fad
> server/src/com/cloud/configuration/ConfigurationManager.java 8db037b
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java 59e70cf
> server/src/com/cloud/storage/StorageManager.java d49a7f8
> server/src/com/cloud/storage/StorageManagerImpl.java d38b35e
> server/src/com/cloud/storage/VolumeManagerImpl.java 43f3681
> server/src/com/cloud/test/DatabaseConfig.java 70c8178
> server/test/com/cloud/vpc/MockConfigurationManagerImpl.java 21b3590
> setup/db/db/schema-410to420.sql bcfbcc9
> ui/dictionary.jsp a5f0662
> ui/scripts/configuration.js cb15598
>
> Diff: https://reviews.apache.org/r/11782/diff/
>
>
> Testing
> -------
>
> testing ok.
>
>
> Thanks,
>
> Wei Zhou
>
>
