You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Wei Zhou <w....@leaseweb.com> on 2013/06/10 19:51:11 UTC
Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11782/
-----------------------------------------------------------
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
ui/scripts/instances.js 7149815
Diff: https://reviews.apache.org/r/11782/diff/
Testing
-------
testing ok.
Thanks,
Wei Zhou
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by Wei Zhou <w....@leaseweb.com>.
> On June 11, 2013, 10:30 p.m., Wido den Hollander wrote:
> >
Wido,
The first and third are debugging messages. For the first one, I will remove the line before this. For the third one, I will remove this line.
For the second one, I will use the methods for coding. Thank you very much for your review.
-Wei
- Wei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11782/#review21754
-----------------------------------------------------------
On June 10, 2013, 5:51 p.m., Wei Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11782/
> -----------------------------------------------------------
>
> (Updated June 10, 2013, 5:51 p.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
> ui/scripts/instances.js 7149815
>
> Diff: https://reviews.apache.org/r/11782/diff/
>
>
> Testing
> -------
>
> testing ok.
>
>
> Thanks,
>
> Wei Zhou
>
>
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by Wido den Hollander <wi...@widodh.nl>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11782/#review21754
-----------------------------------------------------------
api/src/org/apache/cloudstack/api/command/admin/offering/CreateDiskOfferingCmd.java
<https://reviews.apache.org/r/11782/#comment44947>
Any reason this is commented? Or just a small mistake?
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java
<https://reviews.apache.org/r/11782/#comment44950>
Isn't there a way to get this information via the libvirt-java API? If not, what are you missing?
I'm not a big fan of these constructions.
http://libvirt.org/sources/java/javadoc/org/libvirt/Connect.html
Wouldn't these methods help you?
* getCapabilities()
* getHypervisorVersion(java.lang.String type)
* getLibVirVersion()
* getVersion()
server/src/com/cloud/configuration/ConfigurationManagerImpl.java
<https://reviews.apache.org/r/11782/#comment44945>
Why is this warn and nog debug? Also, couldn't you extend these lines a bit more? Since a non-developer would never get what you mean with this.
- Wido den Hollander
On June 10, 2013, 5:51 p.m., Wei Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11782/
> -----------------------------------------------------------
>
> (Updated June 10, 2013, 5:51 p.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
> ui/scripts/instances.js 7149815
>
> Diff: https://reviews.apache.org/r/11782/diff/
>
>
> Testing
> -------
>
> testing ok.
>
>
> Thanks,
>
> Wei Zhou
>
>
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by ASF Subversion and Git Services <as...@urd.zones.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11782/#review22058
-----------------------------------------------------------
Commit 882220e802bb449081ad454251c856e1cd0d7fda in branch refs/heads/master from Wei Zhou
[ https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;h=882220e ]
CLOUDSTACK-1301: VM Disk I/O Throttling
- ASF Subversion and Git Services
On June 17, 2013, 12:03 p.m., Wei Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11782/
> -----------------------------------------------------------
>
> (Updated June 17, 2013, 12:03 p.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 f90edd8
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java b8645e1
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java e91e347
> 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 131d340
> server/src/com/cloud/storage/StorageManager.java d49a7f8
> server/src/com/cloud/storage/StorageManagerImpl.java d38b35e
> server/src/com/cloud/storage/VolumeManagerImpl.java 4297efb
> 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 cadde8c
>
> Diff: https://reviews.apache.org/r/11782/diff/
>
>
> Testing
> -------
>
> testing ok.
>
>
> Thanks,
>
> Wei Zhou
>
>
Re: Review Request 11782: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by ASF Subversion and Git Services <as...@urd.zones.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11782/#review23373
-----------------------------------------------------------
Commit 27b5085542cbc881ea1847e9b894d7723dc869db in branch refs/heads/4.2 from Wei Zhou
[ https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;h=27b5085 ]
CLOUDSTACK-1301: fixed issues and add fields descriptions for disk I/O throttling
- ASF Subversion and Git Services
On June 17, 2013, 12:03 p.m., Wei Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11782/
> -----------------------------------------------------------
>
> (Updated June 17, 2013, 12:03 p.m.)
>
>
> Review request for cloudstack, John Burwell and Wido den Hollander.
>
>
> Bugs: CLOUDSTACK-1301
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> The patch for VM Disk I/O throttling based on commit 3f3c6aa35f64c4129c203d54840524e6aa2c4621
>
>
> 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 f90edd8
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java b8645e1
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java e91e347
> 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 131d340
> server/src/com/cloud/storage/StorageManager.java d49a7f8
> server/src/com/cloud/storage/StorageManagerImpl.java d38b35e
> server/src/com/cloud/storage/VolumeManagerImpl.java 4297efb
> 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 cadde8c
>
> Diff: https://reviews.apache.org/r/11782/diff/
>
>
> Testing
> -------
>
> testing ok.
>
>
> Thanks,
>
> Wei Zhou
>
>
Re: Review Request 11782: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by ASF Subversion and Git Services <as...@urd.zones.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11782/#review23304
-----------------------------------------------------------
Commit bfe76b44daafe7f7844531ef53ee264c2e71c2ea in branch refs/heads/ldapplugin from Wei Zhou
[ https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;h=bfe76b4 ]
CLOUDSTACK-1301: fixed issues and add fields descriptions for disk I/O throttling
- ASF Subversion and Git Services
On June 17, 2013, 12:03 p.m., Wei Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11782/
> -----------------------------------------------------------
>
> (Updated June 17, 2013, 12:03 p.m.)
>
>
> Review request for cloudstack, John Burwell and Wido den Hollander.
>
>
> Bugs: CLOUDSTACK-1301
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> The patch for VM Disk I/O throttling based on commit 3f3c6aa35f64c4129c203d54840524e6aa2c4621
>
>
> 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 f90edd8
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java b8645e1
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java e91e347
> 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 131d340
> server/src/com/cloud/storage/StorageManager.java d49a7f8
> server/src/com/cloud/storage/StorageManagerImpl.java d38b35e
> server/src/com/cloud/storage/VolumeManagerImpl.java 4297efb
> 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 cadde8c
>
> Diff: https://reviews.apache.org/r/11782/diff/
>
>
> Testing
> -------
>
> testing ok.
>
>
> Thanks,
>
> Wei Zhou
>
>
Re: Review Request 11782: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by ASF Subversion and Git Services <as...@urd.zones.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11782/#review23276
-----------------------------------------------------------
Commit bfe76b44daafe7f7844531ef53ee264c2e71c2ea in branch refs/heads/master from Wei Zhou
[ https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;h=bfe76b4 ]
CLOUDSTACK-1301: fixed issues and add fields descriptions for disk I/O throttling
- ASF Subversion and Git Services
On June 17, 2013, 12:03 p.m., Wei Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11782/
> -----------------------------------------------------------
>
> (Updated June 17, 2013, 12:03 p.m.)
>
>
> Review request for cloudstack, John Burwell and Wido den Hollander.
>
>
> Bugs: CLOUDSTACK-1301
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> The patch for VM Disk I/O throttling based on commit 3f3c6aa35f64c4129c203d54840524e6aa2c4621
>
>
> 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 f90edd8
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java b8645e1
> plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java e91e347
> 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 131d340
> server/src/com/cloud/storage/StorageManager.java d49a7f8
> server/src/com/cloud/storage/StorageManagerImpl.java d38b35e
> server/src/com/cloud/storage/VolumeManagerImpl.java 4297efb
> 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 cadde8c
>
> Diff: https://reviews.apache.org/r/11782/diff/
>
>
> Testing
> -------
>
> testing ok.
>
>
> Thanks,
>
> Wei Zhou
>
>
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by Wei Zhou <w....@leaseweb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11782/
-----------------------------------------------------------
(Updated June 17, 2013, 12:03 p.m.)
Review request for cloudstack, Wido den Hollander and John Burwell.
Changes
-------
According to John's comments,
(1) change default value of rates from 0 to null.
Thanks, John.
Description
-------
The patch for VM Disk I/O throttling based on commit 3f3c6aa35f64c4129c203d54840524e6aa2c4621
This addresses bug CLOUDSTACK-1301.
Diffs (updated)
-----
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 f90edd8
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java b8645e1
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtVMDef.java e91e347
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 131d340
server/src/com/cloud/storage/StorageManager.java d49a7f8
server/src/com/cloud/storage/StorageManagerImpl.java d38b35e
server/src/com/cloud/storage/VolumeManagerImpl.java 4297efb
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 cadde8c
Diff: https://reviews.apache.org/r/11782/diff/
Testing
-------
testing ok.
Thanks,
Wei Zhou
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by Wei Zhou <w....@leaseweb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11782/
-----------------------------------------------------------
(Updated June 13, 2013, 9:25 p.m.)
Review request for cloudstack, Wido den Hollander and John Burwell.
Changes
-------
According to John's comments,
(1) use Long instead of long.
(2) wrap blocks with curly braces in DatabaseConfig.java.
Description
-------
The patch for VM Disk I/O throttling based on commit 3f3c6aa35f64c4129c203d54840524e6aa2c4621
This addresses bug CLOUDSTACK-1301.
Diffs (updated)
-----
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 b684e01
server/src/com/cloud/storage/StorageManager.java d49a7f8
server/src/com/cloud/storage/StorageManagerImpl.java d38b35e
server/src/com/cloud/storage/VolumeManagerImpl.java 4297efb
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
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by Wei Zhou <w....@leaseweb.com>.
> 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.
>
> Wei Zhou wrote:
> KVM is the first implementation.
I will use Long instead of long.
- 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
>
>
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by Wei Zhou <w....@leaseweb.com>.
> On June 13, 2013, 6:01 p.m., John Burwell 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
> On June 13, 2013, 6:01 p.m., John Burwell wrote:
> > server/src/com/cloud/test/DatabaseConfig.java, line 923
> > <https://reviews.apache.org/r/11782/diff/2/?file=304095#file304095line923>
> >
> > Coding standard is to wrap all if blocks with curly braces.
I will change it when merge.
- 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
>
>
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by Wei Zhou <w....@leaseweb.com>.
> 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
>
>
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by John Burwell <jb...@basho.com>.
> 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
>
>
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by Wei Zhou <w....@leaseweb.com>.
> 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 ...
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.
- 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
>
>
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by John Burwell <jb...@basho.com>.
> 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,
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.
- 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
>
>
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by John Burwell <jb...@basho.com>.
> 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
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 ...
- 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
>
>
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by John Burwell <jb...@basho.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request: (CLOUDSTACK-1301) VM Disk I/O Throttling
Posted by Wei Zhou <w....@leaseweb.com>.
-----------------------------------------------------------
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.
Changes
-------
According to Wido's comments
(1) remove unused lines.
(2) replace "virsh version" command with conn.getVersion/getLibvirVersion
(3) replace tab with space
Description
-------
The patch for VM Disk I/O throttling based on commit 3f3c6aa35f64c4129c203d54840524e6aa2c4621
This addresses bug CLOUDSTACK-1301.
Diffs (updated)
-----
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