> On Feb. 26, 2013, 2:58 a.m., mice xia wrote: > > server/src/com/cloud/storage/VolumeManagerImpl.java, line 979 > > <https://reviews.apache.org/r/9541/diff/3/?file=261677#file261677line979> > > > > consider to make it transactional? just like persistVolume
incrementResourceCount is internally calling the updateResourceCountForAccount method, which is transactional. > On Feb. 26, 2013, 2:58 a.m., mice xia wrote: > > server/src/com/cloud/storage/VolumeManagerImpl.java, line 1013 > > <https://reviews.apache.org/r/9541/diff/3/?file=261677#file261677line1013> > > > > consider to make it transactional? decrementResourceCount is calling updateResourceCountForAccount, which is transactional. > On Feb. 26, 2013, 2:58 a.m., mice xia wrote: > > server/src/com/cloud/storage/VolumeManagerImpl.java, line 1334 > > <https://reviews.apache.org/r/9541/diff/3/?file=261677#file261677line1334> > > > > consider to make it transactional? this method is internally calling transactional method. > On Feb. 26, 2013, 2:58 a.m., mice xia wrote: > > server/src/com/cloud/storage/VolumeManagerImpl.java, line 1383 > > <https://reviews.apache.org/r/9541/diff/3/?file=261677#file261677line1383> > > > > consider to make it transactional? this method is internally calling transactional method. > On Feb. 26, 2013, 2:58 a.m., mice xia wrote: > > server/src/com/cloud/storage/VolumeManagerImpl.java, line 1456 > > <https://reviews.apache.org/r/9541/diff/3/?file=261677#file261677line1456> > > > > check vol != null ? Missed between volume and vol names,thanks for pointing it out. > On Feb. 26, 2013, 2:58 a.m., mice xia wrote: > > server/src/com/cloud/storage/VolumeManagerImpl.java, line 1466 > > <https://reviews.apache.org/r/9541/diff/3/?file=261677#file261677line1466> > > > > should also add check in resizeVolume method (which is also a new > > feature) Missed this because this API is not present in the API doc i.e. http://incubator.apache.org/cloudstack/docs/api/apidocs-4.0.0/TOC_Root_Admin.html Will update the patch with these changes. On Feb. 26, 2013, 2:58 a.m., Sanjay Tripathi wrote: > > only took a look at VolumeManagerImpl.java and added some suggestions. > > The limit for primary storage refers to allocated_storage , can we make it > > a bit clear (either in code comments or resource.type, maybe one day > > used_storage become another limit) > > > > Yes, it is allocated_storage. Added couple of comments in ResourceType, where these resources are defined. - Sanjay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9541/#review17071 ----------------------------------------------------------- On Feb. 25, 2013, 12:47 p.m., Sanjay Tripathi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9541/ > ----------------------------------------------------------- > > (Updated Feb. 25, 2013, 12:47 p.m.) > > > Review request for cloudstack, Devdeep Singh, mice xia, and Min Chen. > > > Description > ------- > > CLOUDSTACK-1156: Limit Primary and Secondary storage for domain/accounts > > Addition of two new resource types i.e. Primary and Secondary storage > space in the existing pool of > resource types. > Added methods to set the limits on these resources using > updateResourceLimit > API command and to get a count using updateResourceCount. Also added > calls in the > Templates, Volumes, Snapshots life cycle to check these limits and to > increment/decrement the new > resource types > > Resource Name :: Resource type number > Primary Storage 10 > Secondary Storage 11 > > Also added jUnit Tests for the same. > > > This addresses bug CLOUDSTACK-1156. > > > Diffs > ----- > > api/src/com/cloud/configuration/Resource.java 7614c8a > > api/src/org/apache/cloudstack/api/command/user/resource/UpdateResourceCountCmd.java > f6d3a98 > > api/src/org/apache/cloudstack/api/command/user/resource/UpdateResourceLimitCmd.java > 0039f62 > api/src/org/apache/cloudstack/api/response/AccountResponse.java 9a98a35 > api/src/org/apache/cloudstack/api/response/ResourceCountResponse.java > a7fbbf2 > api/src/org/apache/cloudstack/api/response/ResourceLimitResponse.java > b444e7a > server/src/com/cloud/api/ApiResponseHelper.java eafee8a > server/src/com/cloud/api/query/dao/AccountJoinDaoImpl.java 898bafc > server/src/com/cloud/api/query/vo/AccountJoinVO.java cd7231c > server/src/com/cloud/baremetal/BareMetalTemplateAdapter.java 0cf19fb > server/src/com/cloud/configuration/Config.java 8c77715 > server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java 7ff06af > server/src/com/cloud/storage/VolumeManagerImpl.java a69607f > server/src/com/cloud/storage/dao/SnapshotDao.java 0e378a7 > server/src/com/cloud/storage/dao/SnapshotDaoImpl.java 5b3f273 > server/src/com/cloud/storage/dao/VolumeDao.java d7a2667 > server/src/com/cloud/storage/dao/VolumeDaoImpl.java ca3b82a > server/src/com/cloud/storage/download/DownloadMonitorImpl.java 1fd1996 > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java ed48bd1 > server/src/com/cloud/template/HyervisorTemplateAdapter.java ad41af5 > server/src/com/cloud/template/TemplateManagerImpl.java 29659d3 > server/src/com/cloud/vm/UserVmManagerImpl.java cf9eb27 > server/test/com/cloud/resourcelimit/ResourceLimitManagerImplTest.java > d311ad3 > server/test/com/cloud/vpc/MockResourceLimitManagerImpl.java b9fc861 > setup/db/db/schema-40to410.sql 74f0dba > setup/db/db/schema-410to420.sql 4637b6d > utils/src/com/cloud/utils/UriUtils.java a8b5ccb > > Diff: https://reviews.apache.org/r/9541/diff/ > > > Testing > ------- > > Tested life cycle of templates, volumes, snapshots, vm on my local CloudStack > setup. > > > Thanks, > > Sanjay Tripathi > >
