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
[openstack-dev] [Neutron][LBaaS] Improvements to current reviews
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
