> 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.
>
> John Burwell wrote:
> 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 Burwell wrote:
> Is this a KVM specific feature or is KVM the first implementation? If
> this is the first implementation, how well do these new attributes map to the
> other hypervisors? If this feature is KVM specific, I am bit concerned about
> adding attributes and business rules to the Hypervisor layer that are
> specific to a particular hypervisor.
KVM is the first implementation.
- Wei
-----------------------------------------------------------
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
>
>
