cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Burwell" <jburw...@basho.com>
Subject Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Date Thu, 13 Jun 2013 18:01:39 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11782/#review21865
-----------------------------------------------------------



api/src/com/cloud/agent/api/to/VolumeTO.java
<https://reviews.apache.org/r/11782/#comment45155>

    Since the bytesReadRate, bytesWriteRate, iopsReadRate, iopsWriteRate are optional, how
are they represented when they are not specified?  Given that the type is long, I am concerned
that a magic value is being used.  Consider changing the type to Long which will allow null
to be used to clearly represent the optional nature of the value.



api/src/com/cloud/vm/DiskProfile.java
<https://reviews.apache.org/r/11782/#comment45150>

    How are these rate value represented when they are not specified?  I am concerned that
magic values are used in this scenario.  Consider using a Long instead of a long would allow
a clear expression of optionality of the value.



api/src/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java
<https://reviews.apache.org/r/11782/#comment45156>

    See note above about magic values.  It appears that 0 is being used as a magic value to
represent no value specified.



server/src/com/cloud/test/DatabaseConfig.java
<https://reviews.apache.org/r/11782/#comment45157>

    Coding standard is to wrap all if blocks with curly braces.



setup/db/db/schema-410to420.sql
<https://reviews.apache.org/r/11782/#comment45158>

    Concerned again about using 0 as a magic value to represent no value specified.  Consider
converting a nullable column with a default value of null since null is the natural idiom
for representing optional data.


- John Burwell


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