Re: [openstack-dev] [Neutron][LBaaS] Improvements to current reviews

2014-08-11 Thread Susanne Balle
I agree with Doug as well. We should update the current patch. Susanne


On Sun, Aug 10, 2014 at 8:18 AM, Gary Kotton gkot...@vmware.com 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 vijay.venkatacha...@citrix.com
 Reply-To: OpenStack List openstack-dev@lists.openstack.org
 Date: Sunday, August 10, 2014 at 2:57 PM
 To: OpenStack List 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 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 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
  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

2014-08-10 Thread Avishay Balderman
 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 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
 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

2014-08-10 Thread Vijay Venkatachalam
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 
do...@a10networks.commailto: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 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
 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

2014-08-10 Thread Gary Kotton
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 
vijay.venkatacha...@citrix.commailto:vijay.venkatacha...@citrix.com
Reply-To: OpenStack List 
openstack-dev@lists.openstack.orgmailto:openstack-dev@lists.openstack.org
Date: Sunday, August 10, 2014 at 2:57 PM
To: OpenStack List 
openstack-dev@lists.openstack.orgmailto: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 
do...@a10networks.commailto: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 
 brandon.lo...@rackspace.commailto: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.orgmailto:OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.orgmailto: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

2014-08-09 Thread Doug Wiegley
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 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
 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