Re: [openstack-dev] [Cinder][Glance] OSLO update
On 21/11/13 16:14 -0500, Doug Hellmann wrote: So I would really appreciate any comments or pieces of advice. Is it sufficient to include just the short form of the original commit message, along with the commit id in the oslo-incubator repository for reference? I've done this and alse seen it being done by others. In most of the cases, using the commit message title is enough. However, if the sync is intended to fix a bug or introduces more relevant changes, it is definitely useful to have that expressed in the commit message. FF -- @flaper87 Flavio Percoco pgpLu2NRP0IXZ.pgp Description: PGP signature ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Cinder][Glance] OSLO update
But what if I want to update some module that consists of ten or even more files (like rpc or db) and each of these files has quite a long change log? In that case the commit message may turn out to be really long even if only commit ids and names are included. On Wed, Nov 20, 2013 at 12:37 PM, Elena Ezhova eezh...@mirantis.com wrote: 20.11.2013, 06:18, John Griffith john.griff...@solidfire.com: On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin mar...@redhat.com wrote: On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote: Random OSLO updates with no list of what changed, what got fixed etc are unlikely to get review attention - doing such a review is extremely difficult. I was -2ing them and asking for more info, but they keep popping up. I'm really not sure what the best way of updating from OSLO is, but this isn't it. Best practice is to include a list of changes being synced, for example: https://review.openstack.org/54660 Every so often, we throw around ideas for automating the generation of this changes list - e.g. cinder would have the oslo-incubator commit ID for each module stored in a file in git to tell us when it was last synced. Mark. _ __ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev Been away on vacation so I'm afraid I'm a bit late on this... but; I think the point Duncan is bringing up here is that there are some VERY large and significant patches coming from OSLO pulls. The DB patch in particular being over 1K lines of code to a critical portion of the code is a bit unnerving to try and do a review on. I realize that there's a level of trust that goes with the work that's done in OSLO and synchronizing those changes across the projects, but I think a few key concerns here are: 1. Doing huge pulls from OSLO like the DB patch here are nearly impossible to thoroughly review and test. Over time we learn a lot about real usage scenarios and the database and tweak things as we go, so seeing a patch set like this show up is always a bit unnerving and frankly nobody is overly excited to review it. 2. Given a certain level of *trust* for the work that folks do on the OSLO side in submitting these patches and new additions, I think some of the responsibility on the review of the code falls on the OSLO team. That being said there is still the issue of how these changes will impact projects *other* than Nova which I think is sometimes neglected. There have been a number of OSLO synchs pushed to Cinder that fail gating jobs, some get fixed, some get abandoned, but in either case it shows that there wasn't any testing done with projects other than Nova (PLEASE note, I'm not referring to this particular round of patches or calling any patch set out, just stating a historical fact). 3. We need better documentation in commit messages explaining why the changes are necessary and what they do for us. I'm sorry but in my opinion the answer it's the latest in OSLO and Nova already has it is not enough of an answer in my opinion. The patches mentioned in this thread in my opinion met the minimum requirements because they at least reference the OSLO commit which is great. In addition I'd like to see something to address any discovered issues or testing done with the specific projects these changes are being synced to. I'm in no way saying I don't want Cinder to play nice with the common code or to get in line with the way other projects do things but I am saying that I think we have a ways to go in terms of better communication here and in terms of OSLO code actually keeping in mind the entire OpenStack eco-system as opposed to just changes that were needed/updated in Nova. Cinder in particular went through some pretty massive DB re-factoring and changes during Havana and there was a lot of really good work there but it didn't come without a cost and the benefits were examined and weighed pretty heavily. I also think that some times the indirection introduced by adding some of the openstack.common code is unnecessary and in some cases makes things more difficult than they should be. I'm just not sure that we always do a very good ROI investigation or risk assessment on changes, and that opinion applies to ALL changes in OpenStack projects, not OSLO specific or anything else. All of that being said, a couple of those syncs on the list are outdated. We should start by doing a fresh pull for these and if possible add some better documentation in the commit messages as to the justification for the patches that would be great. We can take a closer look at the changes and the history behind them and try to get some review progress made here. Mark mentioned some good ideas regarding capturing commit ID's from synchronization pulls and I'd like to
Re: [openstack-dev] [Cinder][Glance] OSLO update
On 22 November 2013 12:27, Elena Ezhova eezh...@mirantis.com wrote: But what if I want to update some module that consists of ten or even more files (like rpc or db) and each of these files has quite a long change log? In that case the commit message may turn out to be really long even if only commit ids and names are included. A message that is too long is definitely a better problem to have than a message that is missing important details. To turn the question round, how can a reviewer review a change that includes ten or even more files without any information on what changed and why? rpc and db are extremely difficult imports to review, and I've found problems in the last two I looked at. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Cinder][Glance] OSLO update
On Fri, Nov 22, 2013 at 11:21 AM, Duncan Thomas duncan.tho...@gmail.comwrote: On 22 November 2013 12:27, Elena Ezhova eezh...@mirantis.com wrote: But what if I want to update some module that consists of ten or even more files (like rpc or db) and each of these files has quite a long change log? In that case the commit message may turn out to be really long even if only commit ids and names are included. A message that is too long is definitely a better problem to have than a message that is missing important details. If we are talking about merging only part of oslo into a consuming project, then we can't just keep track of the last revision that was merged, because that won't necessarily include all of the changes. Elena, were you planning to keep a separate revision for each entry under openstack/common (not every file, just the files and directories at that level)? To turn the question round, how can a reviewer review a change that includes ten or even more files without any information on what changed and why? rpc and db are extremely difficult imports to review, and I've found problems in the last two I looked at. Problems in the code, or in the way the code was merged? Doug ___ 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] [Cinder][Glance] OSLO update
On 11/20/2013 07:04 AM, Roman Bogorodskiy wrote: I know it was brought up on the list a number of times, but... If we're talking about storing commit ids for each module and writing some shell scripts for that, isn't it a chance to reconsider using git submodules? No. They're too complex. We don't allow merge commits because they're too easy to mess up. Even seasoned (and I mean SEASONED git devs) shy away from submodules because the semantics are tricky. We have 1600 devs - advanced git features lead us to death. (I say this as the person who fields questions about basic git features) On Wed, Nov 20, 2013 at 12:37 PM, Elena Ezhova eezh...@mirantis.com mailto:eezh...@mirantis.com wrote: 20.11.2013, 06:18, John Griffith john.griff...@solidfire.com mailto:john.griff...@solidfire.com: On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin mar...@redhat.com mailto:mar...@redhat.com wrote: On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote: Random OSLO updates with no list of what changed, what got fixed etc are unlikely to get review attention - doing such a review is extremely difficult. I was -2ing them and asking for more info, but they keep popping up. I'm really not sure what the best way of updating from OSLO is, but this isn't it. Best practice is to include a list of changes being synced, for example: https://review.openstack.org/54660 Every so often, we throw around ideas for automating the generation of this changes list - e.g. cinder would have the oslo-incubator commit ID for each module stored in a file in git to tell us when it was last synced. Mark. _ __ 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 Been away on vacation so I'm afraid I'm a bit late on this... but; I think the point Duncan is bringing up here is that there are some VERY large and significant patches coming from OSLO pulls. The DB patch in particular being over 1K lines of code to a critical portion of the code is a bit unnerving to try and do a review on. I realize that there's a level of trust that goes with the work that's done in OSLO and synchronizing those changes across the projects, but I think a few key concerns here are: 1. Doing huge pulls from OSLO like the DB patch here are nearly impossible to thoroughly review and test. Over time we learn a lot about real usage scenarios and the database and tweak things as we go, so seeing a patch set like this show up is always a bit unnerving and frankly nobody is overly excited to review it. 2. Given a certain level of *trust* for the work that folks do on the OSLO side in submitting these patches and new additions, I think some of the responsibility on the review of the code falls on the OSLO team. That being said there is still the issue of how these changes will impact projects *other* than Nova which I think is sometimes neglected. There have been a number of OSLO synchs pushed to Cinder that fail gating jobs, some get fixed, some get abandoned, but in either case it shows that there wasn't any testing done with projects other than Nova (PLEASE note, I'm not referring to this particular round of patches or calling any patch set out, just stating a historical fact). 3. We need better documentation in commit messages explaining why the changes are necessary and what they do for us. I'm sorry but in my opinion the answer it's the latest in OSLO and Nova already has it is not enough of an answer in my opinion. The patches mentioned in this thread in my opinion met the minimum requirements because they at least reference the OSLO commit which is great. In addition I'd like to see something to address any discovered issues or testing done with the specific projects these changes are being synced to. I'm in no way saying I don't want Cinder to play nice with the common code or to get in line with the way other projects do things but I am saying that I think we have a ways to go in terms of better communication here and in terms of OSLO code actually keeping in mind the entire OpenStack eco-system as opposed to just changes that were needed/updated in Nova. Cinder in particular went through some
Re: [openstack-dev] [Cinder][Glance] OSLO update
On Tue, Nov 19, 2013 at 9:12 PM, John Griffith john.griff...@solidfire.comwrote: On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin mar...@redhat.com wrote: On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote: Random OSLO updates with no list of what changed, what got fixed etc are unlikely to get review attention - doing such a review is extremely difficult. I was -2ing them and asking for more info, but they keep popping up. I'm really not sure what the best way of updating from OSLO is, but this isn't it. Best practice is to include a list of changes being synced, for example: https://review.openstack.org/54660 Every so often, we throw around ideas for automating the generation of this changes list - e.g. cinder would have the oslo-incubator commit ID for each module stored in a file in git to tell us when it was last synced. Mark. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev Been away on vacation so I'm afraid I'm a bit late on this... but; I think the point Duncan is bringing up here is that there are some VERY large and significant patches coming from OSLO pulls. The DB patch in particular being over 1K lines of code to a critical portion of the code is a bit unnerving to try and do a review on. I realize that there's a level of trust that goes with the work that's done in OSLO and synchronizing those changes across the projects, but I think a few key concerns here are: 1. Doing huge pulls from OSLO like the DB patch here are nearly impossible to thoroughly review and test. Over time we learn a lot about real usage scenarios and the database and tweak things as we go, so seeing a patch set like this show up is always a bit unnerving and frankly nobody is overly excited to review it. 2. Given a certain level of *trust* for the work that folks do on the OSLO side in submitting these patches and new additions, I think some of the responsibility on the review of the code falls on the OSLO team. That being said there is still the issue of how these changes will impact projects *other* than Nova which I think is sometimes neglected. There have been a number of OSLO synchs pushed to Cinder that fail gating jobs, some get fixed, some get abandoned, but in either case it shows that there wasn't any testing done with projects other than Nova (PLEASE note, I'm not referring to this particular round of patches or calling any patch set out, just stating a historical fact). 3. We need better documentation in commit messages explaining why the changes are necessary and what they do for us. I'm sorry but in my opinion the answer it's the latest in OSLO and Nova already has it is not enough of an answer in my opinion. The patches mentioned in this thread in my opinion met the minimum requirements because they at least reference the OSLO commit which is great. In addition I'd like to see something to address any discovered issues or testing done with the specific projects these changes are being synced to. I'm in no way saying I don't want Cinder to play nice with the common code or to get in line with the way other projects do things but I am saying that I think we have a ways to go in terms of better communication here and in terms of OSLO code actually keeping in mind the entire OpenStack eco-system as opposed to just changes that were needed/updated in Nova. Cinder in particular went through some pretty massive DB re-factoring and changes during Havana and there was a lot of really good work there but it didn't come without a cost and the benefits were examined and weighed pretty heavily. I also think that some times the indirection introduced by adding some of the openstack.common code is unnecessary and in some cases makes things more difficult than they should be. I'm just not sure that we always do a very good ROI investigation or risk assessment on changes, and that opinion applies to ALL changes in OpenStack projects, not OSLO specific or anything else. All of that being said, a couple of those syncs on the list are outdated. We should start by doing a fresh pull for these and if possible add some better documentation in the commit messages as to the justification for the patches that would be great. We can take a closer look at the changes and the history behind them and try to get some review progress made here. Mark mentioned some good ideas regarding capturing commit ID's from synchronization pulls and I'd like to look into that a bit as well. +1 to all of this. We'll work on improving the documentation in commit messages. At the same time, it would be nice to have some of the tweaks and improvements you've made pushed back into Oslo to be shared. The db code in particular is slated to come out of the incubator and become its own library during
Re: [openstack-dev] [Cinder][Glance] OSLO update
On Wed, Nov 20, 2013 at 3:37 AM, Elena Ezhova eezh...@mirantis.com wrote: 20.11.2013, 06:18, John Griffith john.griff...@solidfire.com: On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin mar...@redhat.com wrote: On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote: Random OSLO updates with no list of what changed, what got fixed etc are unlikely to get review attention - doing such a review is extremely difficult. I was -2ing them and asking for more info, but they keep popping up. I'm really not sure what the best way of updating from OSLO is, but this isn't it. Best practice is to include a list of changes being synced, for example: https://review.openstack.org/54660 Every so often, we throw around ideas for automating the generation of this changes list - e.g. cinder would have the oslo-incubator commit ID for each module stored in a file in git to tell us when it was last synced. Mark. _ __ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev Been away on vacation so I'm afraid I'm a bit late on this... but; I think the point Duncan is bringing up here is that there are some VERY large and significant patches coming from OSLO pulls. The DB patch in particular being over 1K lines of code to a critical portion of the code is a bit unnerving to try and do a review on. I realize that there's a level of trust that goes with the work that's done in OSLO and synchronizing those changes across the projects, but I think a few key concerns here are: 1. Doing huge pulls from OSLO like the DB patch here are nearly impossible to thoroughly review and test. Over time we learn a lot about real usage scenarios and the database and tweak things as we go, so seeing a patch set like this show up is always a bit unnerving and frankly nobody is overly excited to review it. 2. Given a certain level of *trust* for the work that folks do on the OSLO side in submitting these patches and new additions, I think some of the responsibility on the review of the code falls on the OSLO team. That being said there is still the issue of how these changes will impact projects *other* than Nova which I think is sometimes neglected. There have been a number of OSLO synchs pushed to Cinder that fail gating jobs, some get fixed, some get abandoned, but in either case it shows that there wasn't any testing done with projects other than Nova (PLEASE note, I'm not referring to this particular round of patches or calling any patch set out, just stating a historical fact). 3. We need better documentation in commit messages explaining why the changes are necessary and what they do for us. I'm sorry but in my opinion the answer it's the latest in OSLO and Nova already has it is not enough of an answer in my opinion. The patches mentioned in this thread in my opinion met the minimum requirements because they at least reference the OSLO commit which is great. In addition I'd like to see something to address any discovered issues or testing done with the specific projects these changes are being synced to. I'm in no way saying I don't want Cinder to play nice with the common code or to get in line with the way other projects do things but I am saying that I think we have a ways to go in terms of better communication here and in terms of OSLO code actually keeping in mind the entire OpenStack eco-system as opposed to just changes that were needed/updated in Nova. Cinder in particular went through some pretty massive DB re-factoring and changes during Havana and there was a lot of really good work there but it didn't come without a cost and the benefits were examined and weighed pretty heavily. I also think that some times the indirection introduced by adding some of the openstack.common code is unnecessary and in some cases makes things more difficult than they should be. I'm just not sure that we always do a very good ROI investigation or risk assessment on changes, and that opinion applies to ALL changes in OpenStack projects, not OSLO specific or anything else. All of that being said, a couple of those syncs on the list are outdated. We should start by doing a fresh pull for these and if possible add some better documentation in the commit messages as to the justification for the patches that would be great. We can take a closer look at the changes and the history behind them and try to get some review progress made here. Mark mentioned some good ideas regarding capturing commit ID's from synchronization pulls and I'd like to look into that a bit as well. Thanks, John ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev I see now that updating OSLO is a
Re: [openstack-dev] [Cinder][Glance] OSLO update
20.11.2013, 06:18, John Griffith john.griff...@solidfire.com: On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin mar...@redhat.com wrote: On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote: Random OSLO updates with no list of what changed, what got fixed etc are unlikely to get review attention - doing such a review is extremely difficult. I was -2ing them and asking for more info, but they keep popping up. I'm really not sure what the best way of updating from OSLO is, but this isn't it. Best practice is to include a list of changes being synced, for example: https://review.openstack.org/54660 Every so often, we throw around ideas for automating the generation of this changes list - e.g. cinder would have the oslo-incubator commit ID for each module stored in a file in git to tell us when it was last synced. Mark. _ __ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev Been away on vacation so I'm afraid I'm a bit late on this... but; I think the point Duncan is bringing up here is that there are some VERY large and significant patches coming from OSLO pulls. The DB patch in particular being over 1K lines of code to a critical portion of the code is a bit unnerving to try and do a review on. I realize that there's a level of trust that goes with the work that's done in OSLO and synchronizing those changes across the projects, but I think a few key concerns here are: 1. Doing huge pulls from OSLO like the DB patch here are nearly impossible to thoroughly review and test. Over time we learn a lot about real usage scenarios and the database and tweak things as we go, so seeing a patch set like this show up is always a bit unnerving and frankly nobody is overly excited to review it. 2. Given a certain level of *trust* for the work that folks do on the OSLO side in submitting these patches and new additions, I think some of the responsibility on the review of the code falls on the OSLO team. That being said there is still the issue of how these changes will impact projects *other* than Nova which I think is sometimes neglected. There have been a number of OSLO synchs pushed to Cinder that fail gating jobs, some get fixed, some get abandoned, but in either case it shows that there wasn't any testing done with projects other than Nova (PLEASE note, I'm not referring to this particular round of patches or calling any patch set out, just stating a historical fact). 3. We need better documentation in commit messages explaining why the changes are necessary and what they do for us. I'm sorry but in my opinion the answer it's the latest in OSLO and Nova already has it is not enough of an answer in my opinion. The patches mentioned in this thread in my opinion met the minimum requirements because they at least reference the OSLO commit which is great. In addition I'd like to see something to address any discovered issues or testing done with the specific projects these changes are being synced to. I'm in no way saying I don't want Cinder to play nice with the common code or to get in line with the way other projects do things but I am saying that I think we have a ways to go in terms of better communication here and in terms of OSLO code actually keeping in mind the entire OpenStack eco-system as opposed to just changes that were needed/updated in Nova. Cinder in particular went through some pretty massive DB re-factoring and changes during Havana and there was a lot of really good work there but it didn't come without a cost and the benefits were examined and weighed pretty heavily. I also think that some times the indirection introduced by adding some of the openstack.common code is unnecessary and in some cases makes things more difficult than they should be. I'm just not sure that we always do a very good ROI investigation or risk assessment on changes, and that opinion applies to ALL changes in OpenStack projects, not OSLO specific or anything else. All of that being said, a couple of those syncs on the list are outdated. We should start by doing a fresh pull for these and if possible add some better documentation in the commit messages as to the justification for the patches that would be great. We can take a closer look at the changes and the history behind them and try to get some review progress made here. Mark mentioned some good ideas regarding capturing commit ID's from synchronization pulls and I'd like to look into that a bit as well. Thanks, John ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev I see now that updating OSLO is a far more complex issue than it may seem from the first sight. But I would really like to do my best
Re: [openstack-dev] [Cinder][Glance] OSLO update
I know it was brought up on the list a number of times, but... If we're talking about storing commit ids for each module and writing some shell scripts for that, isn't it a chance to reconsider using git submodules? On Wed, Nov 20, 2013 at 12:37 PM, Elena Ezhova eezh...@mirantis.com wrote: 20.11.2013, 06:18, John Griffith john.griff...@solidfire.com: On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin mar...@redhat.com wrote: On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote: Random OSLO updates with no list of what changed, what got fixed etc are unlikely to get review attention - doing such a review is extremely difficult. I was -2ing them and asking for more info, but they keep popping up. I'm really not sure what the best way of updating from OSLO is, but this isn't it. Best practice is to include a list of changes being synced, for example: https://review.openstack.org/54660 Every so often, we throw around ideas for automating the generation of this changes list - e.g. cinder would have the oslo-incubator commit ID for each module stored in a file in git to tell us when it was last synced. Mark. _ __ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev Been away on vacation so I'm afraid I'm a bit late on this... but; I think the point Duncan is bringing up here is that there are some VERY large and significant patches coming from OSLO pulls. The DB patch in particular being over 1K lines of code to a critical portion of the code is a bit unnerving to try and do a review on. I realize that there's a level of trust that goes with the work that's done in OSLO and synchronizing those changes across the projects, but I think a few key concerns here are: 1. Doing huge pulls from OSLO like the DB patch here are nearly impossible to thoroughly review and test. Over time we learn a lot about real usage scenarios and the database and tweak things as we go, so seeing a patch set like this show up is always a bit unnerving and frankly nobody is overly excited to review it. 2. Given a certain level of *trust* for the work that folks do on the OSLO side in submitting these patches and new additions, I think some of the responsibility on the review of the code falls on the OSLO team. That being said there is still the issue of how these changes will impact projects *other* than Nova which I think is sometimes neglected. There have been a number of OSLO synchs pushed to Cinder that fail gating jobs, some get fixed, some get abandoned, but in either case it shows that there wasn't any testing done with projects other than Nova (PLEASE note, I'm not referring to this particular round of patches or calling any patch set out, just stating a historical fact). 3. We need better documentation in commit messages explaining why the changes are necessary and what they do for us. I'm sorry but in my opinion the answer it's the latest in OSLO and Nova already has it is not enough of an answer in my opinion. The patches mentioned in this thread in my opinion met the minimum requirements because they at least reference the OSLO commit which is great. In addition I'd like to see something to address any discovered issues or testing done with the specific projects these changes are being synced to. I'm in no way saying I don't want Cinder to play nice with the common code or to get in line with the way other projects do things but I am saying that I think we have a ways to go in terms of better communication here and in terms of OSLO code actually keeping in mind the entire OpenStack eco-system as opposed to just changes that were needed/updated in Nova. Cinder in particular went through some pretty massive DB re-factoring and changes during Havana and there was a lot of really good work there but it didn't come without a cost and the benefits were examined and weighed pretty heavily. I also think that some times the indirection introduced by adding some of the openstack.common code is unnecessary and in some cases makes things more difficult than they should be. I'm just not sure that we always do a very good ROI investigation or risk assessment on changes, and that opinion applies to ALL changes in OpenStack projects, not OSLO specific or anything else. All of that being said, a couple of those syncs on the list are outdated. We should start by doing a fresh pull for these and if possible add some better documentation in the commit messages as to the justification for the patches that would be great. We can take a closer look at the changes and the history behind them and try to get some review progress made here. Mark mentioned some good ideas regarding capturing commit ID's from synchronization pulls and I'd like to look into that a bit as well. Thanks,
Re: [openstack-dev] [Cinder][Glance] OSLO update
On Mon, Nov 18, 2013 at 3:53 PM, Mark McLoughlin mar...@redhat.com wrote: On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote: Random OSLO updates with no list of what changed, what got fixed etc are unlikely to get review attention - doing such a review is extremely difficult. I was -2ing them and asking for more info, but they keep popping up. I'm really not sure what the best way of updating from OSLO is, but this isn't it. Best practice is to include a list of changes being synced, for example: https://review.openstack.org/54660 Every so often, we throw around ideas for automating the generation of this changes list - e.g. cinder would have the oslo-incubator commit ID for each module stored in a file in git to tell us when it was last synced. Mark. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev Been away on vacation so I'm afraid I'm a bit late on this... but; I think the point Duncan is bringing up here is that there are some VERY large and significant patches coming from OSLO pulls. The DB patch in particular being over 1K lines of code to a critical portion of the code is a bit unnerving to try and do a review on. I realize that there's a level of trust that goes with the work that's done in OSLO and synchronizing those changes across the projects, but I think a few key concerns here are: 1. Doing huge pulls from OSLO like the DB patch here are nearly impossible to thoroughly review and test. Over time we learn a lot about real usage scenarios and the database and tweak things as we go, so seeing a patch set like this show up is always a bit unnerving and frankly nobody is overly excited to review it. 2. Given a certain level of *trust* for the work that folks do on the OSLO side in submitting these patches and new additions, I think some of the responsibility on the review of the code falls on the OSLO team. That being said there is still the issue of how these changes will impact projects *other* than Nova which I think is sometimes neglected. There have been a number of OSLO synchs pushed to Cinder that fail gating jobs, some get fixed, some get abandoned, but in either case it shows that there wasn't any testing done with projects other than Nova (PLEASE note, I'm not referring to this particular round of patches or calling any patch set out, just stating a historical fact). 3. We need better documentation in commit messages explaining why the changes are necessary and what they do for us. I'm sorry but in my opinion the answer it's the latest in OSLO and Nova already has it is not enough of an answer in my opinion. The patches mentioned in this thread in my opinion met the minimum requirements because they at least reference the OSLO commit which is great. In addition I'd like to see something to address any discovered issues or testing done with the specific projects these changes are being synced to. I'm in no way saying I don't want Cinder to play nice with the common code or to get in line with the way other projects do things but I am saying that I think we have a ways to go in terms of better communication here and in terms of OSLO code actually keeping in mind the entire OpenStack eco-system as opposed to just changes that were needed/updated in Nova. Cinder in particular went through some pretty massive DB re-factoring and changes during Havana and there was a lot of really good work there but it didn't come without a cost and the benefits were examined and weighed pretty heavily. I also think that some times the indirection introduced by adding some of the openstack.common code is unnecessary and in some cases makes things more difficult than they should be. I'm just not sure that we always do a very good ROI investigation or risk assessment on changes, and that opinion applies to ALL changes in OpenStack projects, not OSLO specific or anything else. All of that being said, a couple of those syncs on the list are outdated. We should start by doing a fresh pull for these and if possible add some better documentation in the commit messages as to the justification for the patches that would be great. We can take a closer look at the changes and the history behind them and try to get some review progress made here. Mark mentioned some good ideas regarding capturing commit ID's from synchronization pulls and I'd like to look into that a bit as well. Thanks, John ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Cinder][Glance] OSLO update
Random OSLO updates with no list of what changed, what got fixed etc are unlikely to get review attention - doing such a review is extremely difficult. I was -2ing them and asking for more info, but they keep popping up. I'm really not sure what the best way of updating from OSLO is, but this isn't it. As an example, the DB pull modifies 1134 lines of code. I see no evidence that the submitter has gone through the ramifications of each line of changed code .v. the rest of the cinder code base, which is what a reviewer needs to do. Just because it changed in OSLO doesn't necessarily mean it will drop straight into cinder. On 14 November 2013 12:21, Elena Ezhova eezh...@mirantis.com wrote: Hello all, I have made several patches that update modules in cinder/openstack/common from oslo which have not been reviewed for more than a month already. My colleague has the same problem with her patches in Glance. Probably it's not a top priority issue, but if oslo is not updated periodically in small bits it may become a problem in the future. What's more, it is much easier for a developer if oslo code is consistent in all projects. So, I would be grateful if someone reviewed these patches: https://review.openstack.org/#/c/48272/ https://review.openstack.org/#/c/48273/ https://review.openstack.org/#/c/52099/ https://review.openstack.org/#/c/52101/ https://review.openstack.org/#/c/53114/ https://review.openstack.org/#/c/47581/ Thanks, Elena ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Duncan Thomas ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Cinder][Glance] OSLO update
On Mon, 2013-11-18 at 17:24 +, Duncan Thomas wrote: Random OSLO updates with no list of what changed, what got fixed etc are unlikely to get review attention - doing such a review is extremely difficult. I was -2ing them and asking for more info, but they keep popping up. I'm really not sure what the best way of updating from OSLO is, but this isn't it. Best practice is to include a list of changes being synced, for example: https://review.openstack.org/54660 Every so often, we throw around ideas for automating the generation of this changes list - e.g. cinder would have the oslo-incubator commit ID for each module stored in a file in git to tell us when it was last synced. Mark. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
[openstack-dev] [Cinder][Glance] OSLO update
Hello all, I have made several patches that update modules in cinder/openstack/common from oslo which have not been reviewed for more than a month already. My colleague has the same problem with her patches in Glance. Probably it's not a top priority issue, but if oslo is not updated periodically in small bits it may become a problem in the future. What's more, it is much easier for a developer if oslo code is consistent in all projects. So, I would be grateful if someone reviewed these patches: https://review.openstack.org/#/c/48272/ https://review.openstack.org/#/c/48273/ https://review.openstack.org/#/c/52099/ https://review.openstack.org/#/c/52101/ https://review.openstack.org/#/c/53114/ https://review.openstack.org/#/c/47581/ Thanks, Elena ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [Cinder][Glance] OSLO update
This ML is not for review requests. Please read http://lists.openstack.org/pipermail/openstack-dev/2013-September/015264.html best, Joe sent on the go On Nov 14, 2013 4:26 AM, Elena Ezhova eezh...@mirantis.com wrote: Hello all, I have made several patches that update modules in cinder/openstack/common from oslo which have not been reviewed for more than a month already. My colleague has the same problem with her patches in Glance. Probably it's not a top priority issue, but if oslo is not updated periodically in small bits it may become a problem in the future. What's more, it is much easier for a developer if oslo code is consistent in all projects. So, I would be grateful if someone reviewed these patches: https://review.openstack.org/#/c/48272/ https://review.openstack.org/#/c/48273/ https://review.openstack.org/#/c/52099/ https://review.openstack.org/#/c/52101/ https://review.openstack.org/#/c/53114/ https://review.openstack.org/#/c/47581/ Thanks, Elena ___ 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
