cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wei Zhou" <w.z...@leaseweb.com>
Subject Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Date Thu, 13 Jun 2013 20:34:42 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message