Re: [openstack-dev] [Neutron][LBaaS] Improvements to current reviews
I agree with Doug as well. We should update the current patch. Susanne On Sun, Aug 10, 2014 at 8:18 AM, Gary Kotton wrote: > Hi, > I took a look at https://review.openstack.org/#/c/105331/ and had one > minor issue which I think can be addressed. Prior to approving we need to > understand what the state of the V2 API will be. > Thanks > Gary > > From: Vijay Venkatachalam > Reply-To: OpenStack List > Date: Sunday, August 10, 2014 at 2:57 PM > To: OpenStack List > > Subject: Re: [openstack-dev] [Neutron][LBaaS] Improvements to current > reviews > > Thanks Brandon for constant improvisation. > > I agree with Doug. Please update current one. We already hv more number > of reviews :-). It will be difficult to manage if we add more. > > Thanks, > Vijay > > Sent using CloudMagic > > On Sun, Aug 10, 2014 at 3:23 AM, Doug Wiegley > wrote: > > I think you should update the current reviews (new patch set, not > additional review.) > > Doug > > > > On Aug 9, 2014, at 3:34 PM, "Brandon Logan" > wrote: > > > > So I've done some work on improving the code on the current pending > > reviews. And would like to get peoples' opinions on whether I should > > add antoher patch set to those reviews, or add the changes as as another > > review dependent on the pending ones. > > > > To be clear, no matter what the first review in the chain will not > > change: > > https://review.openstack.org/#/c/105331/ > > > > However, if adding another patch set is preferrable then plugin and db > > implementation review would get another patch set and then obviously > > anything depending on that. > > > > https://review.openstack.org/#/c/105609/ > > > > My opinion is that I'd like to get both of these in as a new patch set. > > Reason being that the reviews don't have any +2's and there is > > uncertainty because of the GBP discussion. So, I don't think it'd be a > > major issue if a new patch set was created. Speak up if you think > > otherwise. I'd like to get as many people's thoughts as possible. > > > > The changes are: > > > > 1) Added data models, which are just plain python object mimicking the > > sql alchemy models, but don't have the overhead or dynamic nature of > > being sql alchemy models. These data models are now returned by the > > database methods, instead of the sql alchemy objects. Also, I moved the > > definition of the sql alchemy models into its own module. I've been > > wanting to do this but since I thought I was running out of time I left > > it for later. > > > > These shouldn't cause many merge/rebase conflicts, but it probably cause > > a few because the sql alchemy models were moved to a different module. > > > > > > 2) The LoadBalancerPluginv2 no longer inherits from the > > LoadBalancerPluginDbv2. The database is now a composite attribute of > > the plugin (i.e. plugin.db.get_loadbalancer()). This cleans up the code > > a bit and removes the necessity for _delete_db_entity methods that the > > drivers needed because now they can actually call the database methods. > > Also, this makes testing more clear, though I have not added any tests > > for this because the previous tests are sufficient for now. Adding the > > appropriate tests would add 1k or 2k lines most likely. > > > > This will likely cause more conflicts because the _db_delete_entity > > methods have been removed. However, the new driver interface/mixins > > defined a db_method for all drivers to use, so if that is being used > > there shouldn't be any problems. > > > > Thanks, > > Brandon > > > > > > > > > > ___ > > OpenStack-dev mailing list > > OpenStack-dev@lists.openstack.org > > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > > > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > > ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Neutron][LBaaS] Improvements to current reviews
Hi, I took a look at https://review.openstack.org/#/c/105331/ and had one minor issue which I think can be addressed. Prior to approving we need to understand what the state of the V2 API will be. Thanks Gary From: Vijay Venkatachalam mailto:vijay.venkatacha...@citrix.com>> Reply-To: OpenStack List mailto:openstack-dev@lists.openstack.org>> Date: Sunday, August 10, 2014 at 2:57 PM To: OpenStack List mailto:openstack-dev@lists.openstack.org>> Subject: Re: [openstack-dev] [Neutron][LBaaS] Improvements to current reviews Thanks Brandon for constant improvisation. I agree with Doug. Please update current one. We already hv more number of reviews :-). It will be difficult to manage if we add more. Thanks, Vijay Sent using CloudMagic On Sun, Aug 10, 2014 at 3:23 AM, Doug Wiegley mailto:do...@a10networks.com>> wrote: I think you should update the current reviews (new patch set, not additional review.) Doug > On Aug 9, 2014, at 3:34 PM, "Brandon Logan" > mailto:brandon.lo...@rackspace.com>> wrote: > > So I've done some work on improving the code on the current pending > reviews. And would like to get peoples' opinions on whether I should > add antoher patch set to those reviews, or add the changes as as another > review dependent on the pending ones. > > To be clear, no matter what the first review in the chain will not > change: > https://review.openstack.org/#/c/105331/ > > However, if adding another patch set is preferrable then plugin and db > implementation review would get another patch set and then obviously > anything depending on that. > > https://review.openstack.org/#/c/105609/ > > My opinion is that I'd like to get both of these in as a new patch set. > Reason being that the reviews don't have any +2's and there is > uncertainty because of the GBP discussion. So, I don't think it'd be a > major issue if a new patch set was created. Speak up if you think > otherwise. I'd like to get as many people's thoughts as possible. > > The changes are: > > 1) Added data models, which are just plain python object mimicking the > sql alchemy models, but don't have the overhead or dynamic nature of > being sql alchemy models. These data models are now returned by the > database methods, instead of the sql alchemy objects. Also, I moved the > definition of the sql alchemy models into its own module. I've been > wanting to do this but since I thought I was running out of time I left > it for later. > > These shouldn't cause many merge/rebase conflicts, but it probably cause > a few because the sql alchemy models were moved to a different module. > > > 2) The LoadBalancerPluginv2 no longer inherits from the > LoadBalancerPluginDbv2. The database is now a composite attribute of > the plugin (i.e. plugin.db.get_loadbalancer()). This cleans up the code > a bit and removes the necessity for _delete_db_entity methods that the > drivers needed because now they can actually call the database methods. > Also, this makes testing more clear, though I have not added any tests > for this because the previous tests are sufficient for now. Adding the > appropriate tests would add 1k or 2k lines most likely. > > This will likely cause more conflicts because the _db_delete_entity > methods have been removed. However, the new driver interface/mixins > defined a db_method for all drivers to use, so if that is being used > there shouldn't be any problems. > > Thanks, > Brandon > > > > > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org<mailto:OpenStack-dev@lists.openstack.org> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org<mailto:OpenStack-dev@lists.openstack.org> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Neutron][LBaaS] Improvements to current reviews
Thanks Brandon for constant improvisation. I agree with Doug. Please update current one. We already hv more number of reviews :-). It will be difficult to manage if we add more. Thanks, Vijay Sent using CloudMagic On Sun, Aug 10, 2014 at 3:23 AM, Doug Wiegley mailto:do...@a10networks.com>> wrote: I think you should update the current reviews (new patch set, not additional review.) Doug > On Aug 9, 2014, at 3:34 PM, "Brandon Logan" > wrote: > > So I've done some work on improving the code on the current pending > reviews. And would like to get peoples' opinions on whether I should > add antoher patch set to those reviews, or add the changes as as another > review dependent on the pending ones. > > To be clear, no matter what the first review in the chain will not > change: > https://review.openstack.org/#/c/105331/ > > However, if adding another patch set is preferrable then plugin and db > implementation review would get another patch set and then obviously > anything depending on that. > > https://review.openstack.org/#/c/105609/ > > My opinion is that I'd like to get both of these in as a new patch set. > Reason being that the reviews don't have any +2's and there is > uncertainty because of the GBP discussion. So, I don't think it'd be a > major issue if a new patch set was created. Speak up if you think > otherwise. I'd like to get as many people's thoughts as possible. > > The changes are: > > 1) Added data models, which are just plain python object mimicking the > sql alchemy models, but don't have the overhead or dynamic nature of > being sql alchemy models. These data models are now returned by the > database methods, instead of the sql alchemy objects. Also, I moved the > definition of the sql alchemy models into its own module. I've been > wanting to do this but since I thought I was running out of time I left > it for later. > > These shouldn't cause many merge/rebase conflicts, but it probably cause > a few because the sql alchemy models were moved to a different module. > > > 2) The LoadBalancerPluginv2 no longer inherits from the > LoadBalancerPluginDbv2. The database is now a composite attribute of > the plugin (i.e. plugin.db.get_loadbalancer()). This cleans up the code > a bit and removes the necessity for _delete_db_entity methods that the > drivers needed because now they can actually call the database methods. > Also, this makes testing more clear, though I have not added any tests > for this because the previous tests are sufficient for now. Adding the > appropriate tests would add 1k or 2k lines most likely. > > This will likely cause more conflicts because the _db_delete_entity > methods have been removed. However, the new driver interface/mixins > defined a db_method for all drivers to use, so if that is being used > there shouldn't be any problems. > > Thanks, > Brandon > > > > > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Neutron][LBaaS] Improvements to current reviews
" I think you should update the current reviews (new patch set, not additional review.)" +1 I like those changes: +2 -Original Message- From: Doug Wiegley [mailto:do...@a10networks.com] Sent: Sunday, August 10, 2014 12:51 AM To: OpenStack Development Mailing List (not for usage questions) Subject: Re: [openstack-dev] [Neutron][LBaaS] Improvements to current reviews I think you should update the current reviews (new patch set, not additional review.) Doug > On Aug 9, 2014, at 3:34 PM, "Brandon Logan" > wrote: > > So I've done some work on improving the code on the current pending > reviews. And would like to get peoples' opinions on whether I should > add antoher patch set to those reviews, or add the changes as as > another review dependent on the pending ones. > > To be clear, no matter what the first review in the chain will not > change: > https://review.openstack.org/#/c/105331/ > > However, if adding another patch set is preferrable then plugin and db > implementation review would get another patch set and then obviously > anything depending on that. > > https://review.openstack.org/#/c/105609/ > > My opinion is that I'd like to get both of these in as a new patch set. > Reason being that the reviews don't have any +2's and there is > uncertainty because of the GBP discussion. So, I don't think it'd be > a major issue if a new patch set was created. Speak up if you think > otherwise. I'd like to get as many people's thoughts as possible. > > The changes are: > > 1) Added data models, which are just plain python object mimicking the > sql alchemy models, but don't have the overhead or dynamic nature of > being sql alchemy models. These data models are now returned by the > database methods, instead of the sql alchemy objects. Also, I moved > the definition of the sql alchemy models into its own module. I've > been wanting to do this but since I thought I was running out of time > I left it for later. > > These shouldn't cause many merge/rebase conflicts, but it probably > cause a few because the sql alchemy models were moved to a different module. > > > 2) The LoadBalancerPluginv2 no longer inherits from the > LoadBalancerPluginDbv2. The database is now a composite attribute of > the plugin (i.e. plugin.db.get_loadbalancer()). This cleans up the > code a bit and removes the necessity for _delete_db_entity methods > that the drivers needed because now they can actually call the database > methods. > Also, this makes testing more clear, though I have not added any tests > for this because the previous tests are sufficient for now. Adding > the appropriate tests would add 1k or 2k lines most likely. > > This will likely cause more conflicts because the _db_delete_entity > methods have been removed. However, the new driver interface/mixins > defined a db_method for all drivers to use, so if that is being used > there shouldn't be any problems. > > Thanks, > Brandon > > > > > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Neutron][LBaaS] Improvements to current reviews
I think you should update the current reviews (new patch set, not additional review.) Doug > On Aug 9, 2014, at 3:34 PM, "Brandon Logan" > wrote: > > So I've done some work on improving the code on the current pending > reviews. And would like to get peoples' opinions on whether I should > add antoher patch set to those reviews, or add the changes as as another > review dependent on the pending ones. > > To be clear, no matter what the first review in the chain will not > change: > https://review.openstack.org/#/c/105331/ > > However, if adding another patch set is preferrable then plugin and db > implementation review would get another patch set and then obviously > anything depending on that. > > https://review.openstack.org/#/c/105609/ > > My opinion is that I'd like to get both of these in as a new patch set. > Reason being that the reviews don't have any +2's and there is > uncertainty because of the GBP discussion. So, I don't think it'd be a > major issue if a new patch set was created. Speak up if you think > otherwise. I'd like to get as many people's thoughts as possible. > > The changes are: > > 1) Added data models, which are just plain python object mimicking the > sql alchemy models, but don't have the overhead or dynamic nature of > being sql alchemy models. These data models are now returned by the > database methods, instead of the sql alchemy objects. Also, I moved the > definition of the sql alchemy models into its own module. I've been > wanting to do this but since I thought I was running out of time I left > it for later. > > These shouldn't cause many merge/rebase conflicts, but it probably cause > a few because the sql alchemy models were moved to a different module. > > > 2) The LoadBalancerPluginv2 no longer inherits from the > LoadBalancerPluginDbv2. The database is now a composite attribute of > the plugin (i.e. plugin.db.get_loadbalancer()). This cleans up the code > a bit and removes the necessity for _delete_db_entity methods that the > drivers needed because now they can actually call the database methods. > Also, this makes testing more clear, though I have not added any tests > for this because the previous tests are sufficient for now. Adding the > appropriate tests would add 1k or 2k lines most likely. > > This will likely cause more conflicts because the _db_delete_entity > methods have been removed. However, the new driver interface/mixins > defined a db_method for all drivers to use, so if that is being used > there shouldn't be any problems. > > Thanks, > Brandon > > > > > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
