> On Feb. 24, 2014, 7:10 a.m., Koushik Das wrote: > > engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDao.java, > > line 118 > > <https://reviews.apache.org/r/18352/diff/1/?file=499996#file499996line118> > > > > If you specify the host then you don't need dc, pod, cluster.
The reason why I added this was to make sure in case the hostId is passed as null, we could return tagged storagePools by calling findLocalStoragePoolsByTags. This is more of an extra check that we should return all hosts with matching tags even when host is not passed. As suggested it may not be required as there are not other cases where one would call this utility. > On Feb. 24, 2014, 7:10 a.m., Koushik Das wrote: > > engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java, > > line 68 > > <https://reviews.apache.org/r/18352/diff/1/?file=499997#file499997line68> > > > > I would recommend that you use the createSearchBuilder() approach > > instead of hardcoding the sql query. Refer to PrimaryDataStoreDaoImpl() > > ctor. To return local storage pools of matching tags and matching host id requires join of 3 tables (storage_pools, storage_host_ref and storage_pool_details). Further , all the methods that search for tagged pools in this file are currently using the same technique of filling the base sql query with custom search parameters, I have also gone the same way. But as you have mentioned I will update the patch using createSearchBuilder() method. > On Feb. 24, 2014, 7:10 a.m., Koushik Das wrote: > > engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java, > > line 366 > > <https://reviews.apache.org/r/18352/diff/1/?file=499997#file499997line366> > > > > What is the use case for null host id? Make it long instead of Long. This is just because I was checking for null hostIds, since the check may not be required, I will change it to long. - Saksham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18352/#review35257 ----------------------------------------------------------- On Feb. 21, 2014, 11:36 a.m., Saksham Srivastava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18352/ > ----------------------------------------------------------- > > (Updated Feb. 21, 2014, 11:36 a.m.) > > > Review request for cloudstack, Koushik Das and Prachi Damle. > > > Bugs: CLOUDSTACK-6151 > https://issues.apache.org/jira/browse/CLOUDSTACK-6151 > > > Repository: cloudstack-git > > > Description > ------- > > Attaching a disk created from local disk offering with tags, to a VM was > going to wrong local storage pool. > Cause : In LocalStoragePoolAlocator- > List<StoragePoolHostVO> hostPools = > _poolHostDao.listByHostId(plan.getHostId()); > It return pools by hostId, but nowhere were the tags being compared. > > Added new method findLocalStoragePoolsByHostAndTags() that returns stoage > pools by hostid and tags both. > > > Diffs > ----- > > > engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDao.java > 59c338e > > engine/schema/src/org/apache/cloudstack/storage/datastore/db/PrimaryDataStoreDaoImpl.java > d35aa44 > > engine/storage/src/org/apache/cloudstack/storage/allocator/LocalStoragePoolAllocator.java > 1f61e8b > > Diff: https://reviews.apache.org/r/18352/diff/ > > > Testing > ------- > > Tested the folowing scenarios: > attaching local volume with tags > attaching local volume without tags > attaching local volume with different tags > attaching shared volume > > Build passes successfully. > > > Thanks, > > Saksham Srivastava > >
