Yes, looks good. Thanks for fixing this! On Friday, June 20, 2014, Anshul Gangwar <anshul.gang...@citrix.com> wrote:
> I have submitted the patch for this https://reviews.apache.org/r/22807/. > Can somebody have a look into the patch? > > -----Original Message----- > From: Anshul Gangwar [mailto:anshul.gang...@citrix.com <javascript:;>] > Sent: Friday, June 20, 2014 3:32 PM > To: dev@cloudstack.apache.org <javascript:;> > Subject: RE: ZoneWideStoragePoolAllocator#filter seems weird for me > > This patch has introduced the NullPointerException in code. Since > ZoneWideStoragePoolAllocator's filter method has been removed it is using > AbstractStoragePoolAllocator's filter method. > In the filter method there is check for cluster's hypervisor type and and > disk profile hypervisor type. > But cluster is null for ZoneWideStoragePool and Hence the exception. > > Created the issue for same > https://issues.apache.org/jira/browse/CLOUDSTACK-6965. > > Regards, > Anshul > > -----Original Message----- > From: Mike Tutkowski [mailto:mike.tutkow...@solidfire.com <javascript:;>] > Sent: Wednesday, June 18, 2014 10:19 PM > To: dev@cloudstack.apache.org <javascript:;> > Subject: Re: ZoneWideStoragePoolAllocator#filter seems weird for me > > Sounds good - thanks! > > > On Wed, Jun 18, 2014 at 10:46 AM, Yoshikazu Nojima <m...@ynojima.net > <javascript:;>> wrote: > > > Mike, > > > > Thank you for your review. > > This is a patch I would like to push. > > I don't have anything to add regarding this issue. > > I will push this patch in the next few hours. > > > > Regards, > > Noji > > > > > > > > > > 2014-06-18 9:15 GMT-06:00 Mike Tutkowski <mike.tutkow...@solidfire.com > <javascript:;>>: > > > Sorry...I didn't see until later in my e-mail that you had submitted > > code. > > > > > > It looks good to me. > > > > > > Is this something you'd like checked in right away or are you > > > planning to add to this patch? > > > > > > Thanks! > > > > > > > > > On Wed, Jun 18, 2014 at 9:06 AM, Mike Tutkowski < > > > mike.tutkow...@solidfire.com <javascript:;>> wrote: > > > > > >> Yeah, storagePoolHasEnoughIops returns true in the traditional case > > (i.e. > > >> non-managed storage) so we won't skip a storage pool when IOPS are > > >> not really being taken into consideration. > > >> > > >> Feel free to make enhancements and I can review your patch. > > >> > > >> Thanks! > > >> > > >> > > >> On Wed, Jun 18, 2014 at 12:14 AM, Yoshikazu Nojima > > >> <m...@ynojima.net <javascript:;>> > > >> wrote: > > >> > > >>> Prachi, Mike, > > >>> I appreciate your comments. > > >>> > > >>> I think it is a bug that there is a difference between filter > > >>> logic of ClusterScopeStoragePoolAllocator and that of > > >>> ZoneWideStoragePoolAllocator (ex. allocator skip iSCSI type > > >>> storage pool or not when a volume is a RootDisk), I will unify by > > >>> implementing it in AbstractStoragePoolAllocator#filter > > >>> method. > > >>> > > >>> It is new information for me that managed storages only support > > >>> zone level. Thank you for let me know. > > >>> But when iopscapacity parameter is not set to a primary storage (i.e. > > >>> unmanaged storage), storageMgr.storagePoolHasEnoughIops always > > >>> returns true. > > >>> I suppose calling storageMgr.storagePoolHasEnoughIops from > > >>> AbstractStoragePoolAllocator will not cause a problem, and it will > > >>> simplify the code. > > >>> > > >>> I created a patch to explain my thought. Could you review it? > > >>> https://reviews.apache.org/r/22717/ > > >>> > > >>> Regards, > > >>> Noji > > >>> > > >>> > > >>> 2014-06-17 19:19 GMT-06:00 Mike Tutkowski < > > mike.tutkow...@solidfire.com <javascript:;>>: > > >>> > I can comment on the IOPS part. > > >>> > > > >>> > The IOPS are currently only relevant when using managed storage > > (which > > >>> is > > >>> > currently only applicable at the zone level). That being the > > >>> > case, no cluster (or local) storage takes advantage of counting > > >>> > IOPS that have > > >>> been > > >>> > handed out. > > >>> > > > >>> > Of course this logic might be expanded in the future, but that's > > >>> > how > > it > > >>> > works today. > > >>> > > > >>> > > > >>> > On Tue, Jun 17, 2014 at 2:47 PM, Prachi Damle < > > prachi.da...@citrix.com <javascript:;>> > > >>> > wrote: > > >>> > > > >>> >> >Why ZoneWideStoragePoolAllocator implements "filter" method > > >>> >> >(and > > >>> doesn't > > >>> >> call that of base class) rather than just using that of base > class? > > >>> >> >ZoneWideStoragePoolAllocator#filter method seems doesn't care > > "avoid" > > >>> >> parameter, doesn't skip iSCSI type storage pool even if a > > >>> >> volume is > > a > > >>> >> RootDisk. > > >>> >> > > >>> >> The first question seems like a bug to me. > > >>> >> > > >>> >> I am not sure about the IOPS > > >>> >> > > >>> >> Prachi > > >>> >> > > >>> >> -----Original Message----- > > >>> >> From: ynoj...@ynojima.net <javascript:;> [mailto: > ynoj...@ynojima.net <javascript:;>] On > > >>> >> Behalf Of Yoshikazu Nojima > > >>> >> Sent: Tuesday, June 17, 2014 1:07 PM > > >>> >> To: dev@cloudstack.apache.org <javascript:;> > > >>> >> Subject: ZoneWideStoragePoolAllocator#filter seems weird for me > > >>> >> > > >>> >> Hi, > > >>> >> > > >>> >> ZoneWideStoragePoolAllocator#filter seems weird for me. > > >>> >> > > >>> >> ZoneWideStoragePoolAllocator and > > >>> >> ClusterScopeStoragePoolAllocator > > are > > >>> >> allocator classes to select storage pool. > > >>> >> They extends AbstractStoragePoolAllocator class, which provides > > >>> "filter" > > >>> >> method to exclude unavailable storage pools. > > >>> >> > > >>> >> Why ZoneWideStoragePoolAllocator implements "filter" method > > >>> >> (and > > >>> doesn't > > >>> >> call that of base class) rather than just using that of base > class? > > >>> >> > > >>> >> ZoneWideStoragePoolAllocator#filter method seems doesn't care > > "avoid" > > >>> >> parameter, doesn't skip iSCSI type storage pool even if a > > >>> >> volume is > > a > > >>> >> RootDisk. > > >>> >> (These functions are implemented in > > >>> >> AbstractStoragePoolAllocator#filter method, which used by > > >>> >> ClusterScopeStoragePoolAllocator.) > > >>> >> On the other hand, AbstractStoragePoolAllocator#filter doesn't > > >>> >> call storageMgr.storagePoolHasEnoughIops, so a cluster wide > > >>> >> primary > > storage > > >>> >> would be allocated more volumes than its designated IOPS capacity. > > >>> >> > > >>> >> Is there any difference between a zone wide primary storage and > > >>> >> a > > >>> cluster > > >>> >> wide primary storage except its scope? > > >>> >> If it is a bug, I'll fix it. > > >>> >> > > >>> >> Regards, > > >>> >> Noji > > >>> >> > > >>> > > > >>> > > > >>> > > > >>> > -- > > >>> > *Mike Tutkowski* > > >>> > *Senior CloudStack Developer, SolidFire Inc.* > > >>> > e: mike.tutkow...@solidfire.com <javascript:;> > > >>> > o: 303.746.7302 > > >>> > Advancing the way the world uses the cloud > > >>> > <http://solidfire.com/solution/overview/?video=play>*™* > > >>> > > >> > > >> > > >> > > >> -- > > >> *Mike Tutkowski* > > >> *Senior CloudStack Developer, SolidFire Inc.* > > >> e: mike.tutkow...@solidfire.com <javascript:;> > > >> o: 303.746.7302 > > >> Advancing the way the world uses the cloud > > >> <http://solidfire.com/solution/overview/?video=play>*™* > > >> > > > > > > > > > > > > -- > > > *Mike Tutkowski* > > > *Senior CloudStack Developer, SolidFire Inc.* > > > e: mike.tutkow...@solidfire.com <javascript:;> > > > o: 303.746.7302 > > > Advancing the way the world uses the cloud > > > <http://solidfire.com/solution/overview/?video=play>*™* > > > > > > -- > *Mike Tutkowski* > *Senior CloudStack Developer, SolidFire Inc.* > e: mike.tutkow...@solidfire.com <javascript:;> > o: 303.746.7302 > Advancing the way the world uses the cloud > <http://solidfire.com/solution/overview/?video=play>*™* > -- *Mike Tutkowski* *Senior CloudStack Developer, SolidFire Inc.* e: mike.tutkow...@solidfire.com o: 303.746.7302 Advancing the way the world uses the cloud <http://solidfire.com/solution/overview/?video=play>*™*
