No problem, John. I still want to re-review it by myself before coming up with a new patch file.
Also, maybe I should first wait for Wei's changes to be checked in and merge those into mine before generating a new patch file? On Wed, Jun 12, 2013 at 10:40 AM, John Burwell <jburw...@basho.com> wrote: > Mike, > > I just realized that I forgot to publish my review. I am offline ATM, > but I will publish it in the next couple of hours. > > Do you plan to update your the patch in Review Board? > > Sorry for the oversight, > -John > > > > > On Jun 12, 2013, at 2:26 AM, Mike Tutkowski > <mike.tutkow...@solidfire.com> wrote: > > > Hi Edison, John, and Wei (and whoever else is reading this :) ), > > > > Just an FYI that I believe I have implemented all the areas we wanted > > addressed. > > > > I plan to review the code again tomorrow morning or afternoon, then send > in > > another patch. > > > > Thanks for all the work on this everyone! > > > > > > On Tue, Jun 11, 2013 at 12:29 PM, Mike Tutkowski < > > mike.tutkow...@solidfire.com> wrote: > > > >> Sure, that sounds good. > >> > >> > >> On Tue, Jun 11, 2013 at 12:11 PM, Wei ZHOU <ustcweiz...@gmail.com> > wrote: > >> > >>> Hi Mike, > >>> > >>> It looks the two feature do not have many conflicts in Java code, > except > >>> the cloudstack UI. > >>> If you do not mind, I will merge disk_io_throttling branch into master > >>> this > >>> week, so that you can develop based on it. > >>> > >>> -Wei > >>> > >>> > >>> 2013/6/11 Mike Tutkowski <mike.tutkow...@solidfire.com> > >>> > >>>> Hey John, > >>>> > >>>> The SolidFire patch does not depend on the object_store branch, but - > as > >>>> Edison mentioned - it might be easier if we merge the SolidFire branch > >>> into > >>>> the object_store branch before object_store goes into master. > >>>> > >>>> I'm not sure how the disk_io_throttling fits into this merge strategy. > >>>> Perhaps Wei can chime in on that. > >>>> > >>>> > >>>> On Tue, Jun 11, 2013 at 11:07 AM, John Burwell <jburw...@basho.com> > >>> wrote: > >>>> > >>>>> Mike, > >>>>> > >>>>> We have a delicate merge dance to perform. The disk_io_throttling, > >>>>> solidfire, and object_store appear to have a number of overlapping > >>>>> elements. I understand the dependencies between the patches to be as > >>>>> follows: > >>>>> > >>>>> object_store <- solidfire -> disk_io_throttling > >>>>> > >>>>> Am I correct that the device management aspects of SolidFire are > >>> additive > >>>>> to the object_store branch or there are circular dependency between > >>> the > >>>>> branches? Once we understand the dependency graph, we can determine > >>> the > >>>>> best approach to land the changes in master. > >>>>> > >>>>> Thanks, > >>>>> -John > >>>>> > >>>>> > >>>>> On Jun 10, 2013, at 11:10 PM, Mike Tutkowski < > >>>> mike.tutkow...@solidfire.com> > >>>>> wrote: > >>>>> > >>>>>> Also, if we are good with Edison merging my code into his branch > >>> before > >>>>>> going into master, I am good with that. > >>>>>> > >>>>>> We can remove the StoragePoolType.Dynamic code after his merge and > >>> we > >>>> can > >>>>>> deal with Burst IOPS then, as well. > >>>>>> > >>>>>> > >>>>>> On Mon, Jun 10, 2013 at 9:08 PM, Mike Tutkowski < > >>>>>> mike.tutkow...@solidfire.com> wrote: > >>>>>> > >>>>>>> Let me make sure I follow where we're going here: > >>>>>>> > >>>>>>> 1) There should be NO references to hypervisor code in the storage > >>>>>>> plug-ins code (this includes the default storage plug-in, which > >>>>> currently > >>>>>>> sends several commands to the hypervisor in use (although it does > >>> not > >>>>> know > >>>>>>> which hypervisor (XenServer, ESX(i), etc.) is actually in use)) > >>>>>>> > >>>>>>> 2) managed=true or managed=false can be placed in the url field (if > >>>> not > >>>>>>> present, we default to false). This info is stored in the > >>>>>>> storage_pool_details table. > >>>>>>> > >>>>>>> 3) When the "attach" command is sent to the hypervisor in > >>> question, we > >>>>>>> pass the managed property along (this takes the place of the > >>>>>>> StoragePoolType.Dynamic check). > >>>>>>> > >>>>>>> 4) execute(AttachVolumeCommand) in the hypervisor checks for the > >>>> managed > >>>>>>> property. If true for an attach, the necessary hypervisor data > >>>>> structure is > >>>>>>> created and the rest of the attach command executes to attach the > >>>>> volume. > >>>>>>> > >>>>>>> 5) When execute(AttachVolumeCommand) is invoked to detach a volume, > >>>> the > >>>>>>> same check is made. If managed, the hypervisor data structure is > >>>>> removed. > >>>>>>> > >>>>>>> 6) I do not see an clear way to support Burst IOPS in 4.2 unless > >>> it is > >>>>>>> stored in the volumes and disk_offerings table. If we have some > >>> idea, > >>>>>>> that'd be cool. > >>>>>>> > >>>>>>> Thanks! > >>>>>>> > >>>>>>> > >>>>>>> On Mon, Jun 10, 2013 at 8:58 PM, Mike Tutkowski < > >>>>>>> mike.tutkow...@solidfire.com> wrote: > >>>>>>> > >>>>>>>> "+1 -- Burst IOPS can be implemented while avoiding implementation > >>>>>>>> attributes. I always wondered about the details field. I think > >>> we > >>>>> should > >>>>>>>> beef up the description in the documentation regarding the > >>> expected > >>>>> format > >>>>>>>> of the field. In 4.1, I noticed that the details are not > >>> returned on > >>>>> the > >>>>>>>> createStoratePool updateStoragePool, or listStoragePool response. > >>>> Why > >>>>>>>> don't we return it? It seems like it would be useful for clients > >>> to > >>>> be > >>>>>>>> able to inspect the contents of the details field." > >>>>>>>> > >>>>>>>> Not sure how this would work storing Burst IOPS here. > >>>>>>>> > >>>>>>>> Burst IOPS need to be variable on a Disk Offering-by-Disk Offering > >>>>>>>> basis. For each Disk Offering created, you have to be able to > >>>> associate > >>>>>>>> unique Burst IOPS. There is a disk_offering_details table. Maybe > >>> it > >>>>> could > >>>>>>>> go there? > >>>>>>>> > >>>>>>>> I'm also not sure how you would accept the Burst IOPS in the GUI > >>> if > >>>>> it's > >>>>>>>> not stored like the Min and Max fields are in the DB. > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> -- > >>>>>>> *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> > >>>>>>> *™* > >>>>>> > >>>>>> > >>>>>> > >>>>>> -- > >>>>>> *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> > >>>>>> *™* > >>>> > >>>> > >>>> -- > >>>> *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> > >>>> *™* > >> > >> > >> > >> -- > >> *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> > >> *™* > > > > > > > > -- > > *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> > > *™* > -- *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> *™*
