Dear All, There are many occasion when we want to priorities some of the patches whether it is related to unblock the gates or blocking the non freeze patches during RC. So adding the Review-Priority will allow more precise dashboard. As Designate and Cinder projects already experiencing this[1][2] and after discussion with Jeremy brought this to ML to interact with these team before landing [3], as there is possibility that reapply the priority vote following any substantive updates to change could make it more cumbersome than it is worth. Teams please share your experience of using Review-Priority. [1] https://review.openstack.org/#/c/295253/ [2] https://review.openstack.org/#/c/620664/ [3] https://review.openstack.org/#/c/626824/ --- Best Regards Surya
I'm curious about this feedback, too. I have this topic as an agenda item for the next keystone meeting [0], but it would be good to hear what the cinder and designate folks think. [0] https://etherpad.openstack.org/p/keystone-weekly-meeting On Thu, Dec 27, 2018 at 11:37 PM Surya Singh <singh.surya64mnnit@gmail.com> wrote:
Dear All,
There are many occasion when we want to priorities some of the patches whether it is related to unblock the gates or blocking the non freeze patches during RC.
So adding the Review-Priority will allow more precise dashboard. As Designate and Cinder projects already experiencing this[1][2] and after discussion with Jeremy brought this to ML to interact with these team before landing [3], as there is possibility that reapply the priority vote following any substantive updates to change could make it more cumbersome than it is worth.
Teams please share your experience of using Review-Priority.
[1] https://review.openstack.org/#/c/295253/ [2] https://review.openstack.org/#/c/620664/ [3] https://review.openstack.org/#/c/626824/
--- Best Regards Surya
On Fri, Dec 28, 2018 at 11:04:41AM +0530, Surya Singh wrote:
Dear All,
There are many occasion when we want to priorities some of the patches whether it is related to unblock the gates or blocking the non freeze patches during RC.
So adding the Review-Priority will allow more precise dashboard. As Designate and Cinder projects already experiencing this[1][2] and after discussion with Jeremy brought this to ML to interact with these team before landing [3], as there is possibility that reapply the priority vote following any substantive updates to change could make it more cumbersome than it is worth.
With Cinder this is fairly new, but I think it is working well so far. The oddity we've run into, that I think you're referring to here, is how those votes carry forward with updates. I set up Cinder with -1, +1, and +2 as possible priority votes. It appears when a patchset is updates, the -1 and +2 carry forward. But for some reason we can't get the +1 to be sticky. So far, that's just a slight inconvenience. It would be great if we can figure out a way to have them all be sticky, but if we need to live with reapplying +1 votes, that's manageable to me. The one thing I have been slightly concerned with is the process around using these priority votes. It hasn't been an issue, but I could see a scenario where one core (in Cinder we have it set up so all cores can use the priority voting) has set something like a procedural -1, then been pulled away or is absent for an extended period. Like a Workflow -2, another core cannot override that vote. So until that person is back to remove the -1, that patch would not be able to be merged. Granted, we've lived with this with Workflow -2's for years and it's never been a major issue, but I think as far as centralizing control, it may make sense to have a separate smaller group (just the PTL, or PTL and a few "deputies") that are able to set priorities on patches just to make sure the folks setting it are the ones that are actively tracking what the priorities are for the project. Anyway, my 2 cents. I can imagine this would work really well for some teams, less well for others. So if you think it can help you manage your project priorities, I would recommend giving it a shot and seeing how it goes. You can always drop it if it ends up not being effective or causing issues. Sean
On 2019-01-03 07:51:55 -0600 (-0600), Sean McGinnis wrote: [...]
The one thing I have been slightly concerned with is the process around using these priority votes. It hasn't been an issue, but I could see a scenario where one core (in Cinder we have it set up so all cores can use the priority voting) has set something like a procedural -1, then been pulled away or is absent for an extended period. Like a Workflow -2, another core cannot override that vote. So until that person is back to remove the -1, that patch would not be able to be merged. [...]
Please treat it as only a last resort, but the solution to this is that a Gerrit admin (find us in #openstack-infra on Freenode or the openstack-infra ML on lists.openstack.org or here on openstack-discuss with an [infra] subject tag) can selectively delete votes on a change at the request of a project leader (PTL, infra liaison, TC member...) to unblock your work. -- Jeremy Stanley
On 3 Jan 2019, at 5:51, Sean McGinnis wrote:
On Fri, Dec 28, 2018 at 11:04:41AM +0530, Surya Singh wrote:
Dear All,
There are many occasion when we want to priorities some of the patches whether it is related to unblock the gates or blocking the non freeze patches during RC.
So adding the Review-Priority will allow more precise dashboard. As Designate and Cinder projects already experiencing this[1][2] and after discussion with Jeremy brought this to ML to interact with these team before landing [3], as there is possibility that reapply the priority vote following any substantive updates to change could make it more cumbersome than it is worth.
With Cinder this is fairly new, but I think it is working well so far. The oddity we've run into, that I think you're referring to here, is how those votes carry forward with updates.
I set up Cinder with -1, +1, and +2 as possible priority votes. It appears when a patchset is updates, the -1 and +2 carry forward. But for some reason we can't get the +1 to be sticky.
So far, that's just a slight inconvenience. It would be great if we can figure out a way to have them all be sticky, but if we need to live with reapplying +1 votes, that's manageable to me.
The one thing I have been slightly concerned with is the process around using these priority votes. It hasn't been an issue, but I could see a scenario where one core (in Cinder we have it set up so all cores can use the priority voting) has set something like a procedural -1, then been pulled away or is absent for an extended period. Like a Workflow -2, another core cannot override that vote. So until that person is back to remove the -1, that patch would not be able to be merged.
Granted, we've lived with this with Workflow -2's for years and it's never been a major issue, but I think as far as centralizing control, it may make sense to have a separate smaller group (just the PTL, or PTL and a few "deputies") that are able to set priorities on patches just to make sure the folks setting it are the ones that are actively tracking what the priorities are for the project.
Anyway, my 2 cents. I can imagine this would work really well for some teams, less well for others. So if you think it can help you manage your project priorities, I would recommend giving it a shot and seeing how it goes. You can always drop it if it ends up not being effective or causing issues.
Sean
This looks pretty interesting. I have a few question about how it's practically working out. I get the impression that the values of the votes are configurable? So you've chosen -1, +1, and +2, but you could have chosen 1, 2, 3, 4, 5 (for example)? Do you have an example of a dashboard that's using these values? IMO, gerrit's display of votes is rather bad. I'd prefer that votes like this could be aggregated. How do you manage discovering what patches are priority or not? I guess that's where the dashboards come in? I get the impression that particular priority votes have the ability to block a merge. How does that work? half-plug/half-context, I've attempted to solve priority discovery in the Swift community with some customer dashboards. We've got a review dashboard in gerrit [1] that shows "starred by the ptl". I've also created a tool that finds all the starred patches by all contributors, weights each contributor by how much they've contributed recently, and then sorts the resulting totals as a list of "stuff the community thinks is important"[2]. As a community, we also manage our own wiki page for prioritization[3]. I'd love to see if some functionality in gerrit, eg these priority review votes, could supplant some of our other tools. [1] http://not.mn/reviews.html [2] http://d.not.mn/swift_community_dashboard.html [3] https://wiki.openstack.org/wiki/Swift/PriorityReviews --John
This looks pretty interesting. I have a few question about how it's practically working out.
I get the impression that the values of the votes are configurable? So you've chosen -1, +1, and +2, but you could have chosen 1, 2, 3, 4, 5 (for example)?
Yes, I believe so. Maybe someone more familiar with how this works in Gerrit can correct me if I'm misstating that.
Do you have an example of a dashboard that's using these values?
Being able to easily query and create a dashboard for this is the big benefit. Here's what I came up with for Cinder: https://tiny.cc/CinderPriorities
IMO, gerrit's display of votes is rather bad. I'd prefer that votes like this could be aggregated. How do you manage discovering what patches are priority or not? I guess that's where the dashboards come in?
We haven't been aggregating votes, rather just a core can decide to flag things are priority or not.
I get the impression that particular priority votes have the ability to block a merge. How does that work?
I believe this is what controls that: http://git.openstack.org/cgit/openstack-infra/project-config/tree/gerrit/acl... So I believe that could be NoBlock or NoOp to allow just ranking without enforcing any kind of blocking.
On 2019-01-03 13:50:48 -0600 (-0600), Sean McGinnis wrote: [...]
I get the impression that the values of the votes are configurable? So you've chosen -1, +1, and +2, but you could have chosen 1, 2, 3, 4, 5 (for example)?
Yes, I believe so. Maybe someone more familiar with how this works in Gerrit can correct me if I'm misstating that. [...]
The value ranges are entirely arbitrary as far as I know. Keep in mind though that the Gerrit configuration to carry over votes to new patch sets under specific conditions can apparently only be set to carry over the highest and lowest possible values, but none in between. I really don't understand that design choice on their part, but that's what it does.
So I believe that could be NoBlock or NoOp to allow just ranking without enforcing any kind of blocking.
Yes, that should work if it's the behavior you're looking for. -- Jeremy Stanley
<snip>
With Cinder this is fairly new, but I think it is working well so far. The oddity we've run into, that I think you're referring to here, is how those votes carry forward with updates.
</snip> It is unfortunate that we can't get +1's to carry forward but I don't think this negates the value of having the priorities. I have been using our review dashboard quite a bit lately and plan to set up processes that involve it as we move forward to using/documenting Storyboard for Cinder. <snip>
So far, that's just a slight inconvenience. It would be great if we can figure out a way to have them all be sticky, but if we need to live with reapplying +1 votes, that's manageable to me.
</snip> Is there someway that we could allow the owner to reset this priority after pushing up a new patch. That would lower the dependence on the cores in that case. <snip>
Anyway, my 2 cents. I can imagine this would work really well for some teams, less well for others. So if you think it can help you manage your project priorities, I would recommend giving it a shot and seeing how it goes. You can always drop it if it ends up not being effective or causing issues.
Sean
</snip> As usual, the biggest problem I am seeing is getting enough people to do the reviews and really set up all the priorities appropriately. There are just a couple of us doing it right now. I am hoping to see more participation in the coming months to make the output more beneficial for all.
On Thu, Jan 3, 2019, at 4:26 PM, Jay Bryant wrote:
<snip>
snip
So far, that's just a slight inconvenience. It would be great if we can figure out a way to have them all be sticky, but if we need to live with reapplying +1 votes, that's manageable to me.
</snip>
Is there someway that we could allow the owner to reset this priority after pushing up a new patch. That would lower the dependence on the cores in that case.
If you use a three value label: [-1: +1] then you could set copy min and max scores so all values are carried forward on new patchsets. This would allow you to have -1 "Don't review", 0 "default no special priority", and +1 "this is a priority please review now". This may have to take advantage of the fact that if you don't set a value its roughly the same as 0 (I don't know if this is explicitly true in Gerrit but we can approximate it since -1 and +1 would be explicitly set and query on those values). If you need an explicit copy all values function in Gerrit you'll want to get that merged upstream first then we could potentially backport it to our Gerrit. This will likely require writing Java. For some reason I thought that Prolog predicates could be written for these value copying functions, but docs seem to say otherwise. Prolog is only for determining if a label's value allows a change to be submitted (merged). Clark
Clark Boylan <cboylan@sapwetik.org> writes:
On Thu, Jan 3, 2019, at 4:26 PM, Jay Bryant wrote:
<snip>
snip
So far, that's just a slight inconvenience. It would be great if we can figure out a way to have them all be sticky, but if we need to live with reapplying +1 votes, that's manageable to me.
</snip>
Is there someway that we could allow the owner to reset this priority after pushing up a new patch. That would lower the dependence on the cores in that case.
If you use a three value label: [-1: +1] then you could set copy min and max scores so all values are carried forward on new patchsets. This would allow you to have -1 "Don't review", 0 "default no special priority", and +1 "this is a priority please review now". This may have to take advantage of the fact that if you don't set a value its roughly the same as 0 (I don't know if this is explicitly true in Gerrit but we can approximate it since -1 and +1 would be explicitly set and query on those values).
It is possible to tell the difference between not having a value set and having the default set, but as you point out if the dashboards are simply configured to look for +1 and -1 then the other distinction isn't important.
If you need an explicit copy all values function in Gerrit you'll want to get that merged upstream first then we could potentially backport it to our Gerrit. This will likely require writing Java.
We could also have a bot do it. The history of each patch is available, so it's possible to determine that a priority was set but lost when a new patch is submitted. The first step to having a bot would be to write the logic to fix the lost priorities, and if someone does that as a CLI then teams could use that by hand until someone configures the bot.
For some reason I thought that Prolog predicates could be written for these value copying functions, but docs seem to say otherwise. Prolog is only for determining if a label's value allows a change to be submitted (merged).
Clark
-- Doug
---- On Thu, 03 Jan 2019 22:51:55 +0900 Sean McGinnis <sean.mcginnis@gmx.com> wrote ----
On Fri, Dec 28, 2018 at 11:04:41AM +0530, Surya Singh wrote:
Dear All,
There are many occasion when we want to priorities some of the patches whether it is related to unblock the gates or blocking the non freeze patches during RC.
So adding the Review-Priority will allow more precise dashboard. As Designate and Cinder projects already experiencing this[1][2] and after discussion with Jeremy brought this to ML to interact with these team before landing [3], as there is possibility that reapply the priority vote following any substantive updates to change could make it more cumbersome than it is worth.
With Cinder this is fairly new, but I think it is working well so far. The oddity we've run into, that I think you're referring to here, is how those votes carry forward with updates.
I set up Cinder with -1, +1, and +2 as possible priority votes. It appears when
This idea looks great and helpful especially for blockers and cycle priority patches to get regular review bandwidth from Core or Active members of that project. IMO only +ve votes are more appropriate for this label. -1 is little confusing for many reasons like what is the difference between Review-Priority -1 and Code-Review -2 ? Review-Priority -1 means, it is less priority than 0/not labelled (explicitly setting any patch very less priority). After seeing Cinder dashboard, I got to know that -1 is used to block the changes due to procedural or technical reason. But that can be done by -2 on Code-Review label. Keeping Review-Priority label only for priority set makes it more clear which is nothing but allowing only +ve votes for this label. Personally, I prefer only a single vote set which can be +1 to convey that these are the set of changes priority for review but having multiple +ve vote set as per project need/interest is all fine. -gmann
a patchset is updates, the -1 and +2 carry forward. But for some reason we can't get the +1 to be sticky.
So far, that's just a slight inconvenience. It would be great if we can figure out a way to have them all be sticky, but if we need to live with reapplying +1 votes, that's manageable to me.
The one thing I have been slightly concerned with is the process around using these priority votes. It hasn't been an issue, but I could see a scenario where one core (in Cinder we have it set up so all cores can use the priority voting) has set something like a procedural -1, then been pulled away or is absent for an extended period. Like a Workflow -2, another core cannot override that vote. So until that person is back to remove the -1, that patch would not be able to be merged.
Granted, we've lived with this with Workflow -2's for years and it's never been a major issue, but I think as far as centralizing control, it may make sense to have a separate smaller group (just the PTL, or PTL and a few "deputies") that are able to set priorities on patches just to make sure the folks setting it are the ones that are actively tracking what the priorities are for the project.
Anyway, my 2 cents. I can imagine this would work really well for some teams, less well for others. So if you think it can help you manage your project priorities, I would recommend giving it a shot and seeing how it goes. You can always drop it if it ends up not being effective or causing issues.
Sean
Ghanshyam Mann <gmann@ghanshyammann.com> writes:
---- On Thu, 03 Jan 2019 22:51:55 +0900 Sean McGinnis <sean.mcginnis@gmx.com> wrote ----
On Fri, Dec 28, 2018 at 11:04:41AM +0530, Surya Singh wrote:
Dear All,
There are many occasion when we want to priorities some of the patches whether it is related to unblock the gates or blocking the non freeze patches during RC.
So adding the Review-Priority will allow more precise dashboard. As Designate and Cinder projects already experiencing this[1][2] and after discussion with Jeremy brought this to ML to interact with these team before landing [3], as there is possibility that reapply the priority vote following any substantive updates to change could make it more cumbersome than it is worth.
With Cinder this is fairly new, but I think it is working well so far. The oddity we've run into, that I think you're referring to here, is how those votes carry forward with updates.
I set up Cinder with -1, +1, and +2 as possible priority votes. It appears when
This idea looks great and helpful especially for blockers and cycle priority patches to get regular review bandwidth from Core or Active members of that project.
IMO only +ve votes are more appropriate for this label. -1 is little confusing for many reasons like what is the difference between Review-Priority -1 and Code-Review -2 ? Review-Priority -1 means, it is less priority than 0/not labelled (explicitly setting any patch very less priority).
After seeing Cinder dashboard, I got to know that -1 is used to block the changes due to procedural or technical reason. But that can be done by -2 on Code-Review label. Keeping Review-Priority label only for priority set makes it more clear which is nothing but allowing only +ve votes for this label. Personally, I prefer only a single vote set which can be +1 to convey that these are the set of changes priority for review but having multiple +ve vote set as per project need/interest is all fine.
-gmann
Given the complexity of our review process already, if this new aspect is going to spread it would be really nice if we could try to agree on a standard way to apply it. Not only would that let someone build a dashboard for cross-project priorities, but it would mean contributors wouldn't need to learn different rules for interacting with each of our teams. Doug
On 1/10/19 12:17 AM, Ghanshyam Mann wrote:
---- On Thu, 03 Jan 2019 22:51:55 +0900 Sean McGinnis <sean.mcginnis@gmx.com> wrote ----
On Fri, Dec 28, 2018 at 11:04:41AM +0530, Surya Singh wrote:
Dear All,
There are many occasion when we want to priorities some of the patches whether it is related to unblock the gates or blocking the non freeze patches during RC.
So adding the Review-Priority will allow more precise dashboard. As Designate and Cinder projects already experiencing this[1][2] and after discussion with Jeremy brought this to ML to interact with these team before landing [3], as there is possibility that reapply the priority vote following any substantive updates to change could make it more cumbersome than it is worth.
With Cinder this is fairly new, but I think it is working well so far. The oddity we've run into, that I think you're referring to here, is how those votes carry forward with updates.
I set up Cinder with -1, +1, and +2 as possible priority votes. It appears when
This idea looks great and helpful especially for blockers and cycle priority patches to get regular review bandwidth from Core or Active members of that project.
IMO only +ve votes are more appropriate for this label. -1 is little confusing for many reasons like what is the difference between Review-Priority -1 and Code-Review -2 ? Review-Priority -1 means, it is less priority than 0/not labelled (explicitly setting any patch very less priority).
After seeing Cinder dashboard, I got to know that -1 is used to block the changes due to procedural or technical reason. But that can be done by -2 on Code-Review label. Keeping Review-Priority label only for priority set makes it more clear which is nothing but allowing only +ve votes for this label. Personally, I prefer only a single vote set which can be +1 to convey that these are the set of changes priority for review but having multiple +ve vote set as per project need/interest is all fine.
I don't know if this was the reasoning behind Cinder's system, but I know some people object to procedural -2 because it's a big hammer to essentially say "not right now". It overloads the meaning of the vote in a potentially confusing way that requires explanation every time it's used. At least I hope procedural -2's always include a comment. Whether adding a whole new vote type is a meaningful improvement is another question, but if we're adding the type anyway for prioritization it might make sense to use it to replace procedural -2. Especially if we could make it so any core can change it (apparently not right now), whereas -2 requires the original core to come back and remove it.
I don't know if this was the reasoning behind Cinder's system, but I know some people object to procedural -2 because it's a big hammer to essentially say "not right now". It overloads the meaning of the vote in a potentially confusing way that requires explanation every time it's used. At least I hope procedural -2's always include a comment.
This was exactly the reasoning. -2 is overloaded, but its primary meaning was/is "we do not want this code change". It just happens that it was also a convenient way to say that with "right now" at the end. The Review-Priority -1 is a clear way to say whether something is held because it can't be merged right now due to procedural or process reasons, versus something that we just don't want at all.
On Thu, 2019-01-10 at 13:42 -0600, Sean McGinnis wrote:
I don't know if this was the reasoning behind Cinder's system, but I know some people object to procedural -2 because it's a big hammer to essentially say "not right now". It overloads the meaning of the vote in a potentially confusing way that requires explanation every time it's used. At least I hope procedural -2's always include a comment.
This was exactly the reasoning. -2 is overloaded, but its primary meaning was/is "we do not want this code change". It just happens that it was also a convenient way to say that with "right now" at the end.
The Review-Priority -1 is a clear way to say whether something is held because it can't be merged right now due to procedural or process reasons, versus something that we just don't want at all. for what its worth my understanding of why a procdural -2 is more correct is that this change cannot be merged because it has not met the procedual requirement to be considerd for this release. haveing received several over the years i have never seen it to carry any malaise or weight then the zuul pep8 job complianing about the line lenght of my code. with either a procedural -2 or a verify -1 from zuul my code is equally un mergeable.
the prime example being a patch that requires a spec that has not been approved. while most cores will not approve chage when other cores have left a -1 mistakes happen and the -2 does emphasise the point that even if the code is perfect under the porject processes this change should not be acitvly reporposed until the issue raised by the -2 has been addressed. In the case of a procedual -2 that typically means the spec is merge or the master branch opens for the next cycle. i agree that procedural -2's can seam harsh at first glance but i have also never seen one left without a comment explaining why it was left. the issue with a procedural -1 is i can jsut resubmit the patch several times and it can get lost in the comments. we recently intoduced a new review priority lable if we really wanted to disabiguate form normal -2s then we coudl have an explcitly lable for it but i personally would prefer to keep procedural -2s. anyway that just my two cents.
On 1/10/19 5:03 PM, Sean Mooney wrote:
On Thu, 2019-01-10 at 13:42 -0600, Sean McGinnis wrote:
I don't know if this was the reasoning behind Cinder's system, but I know some people object to procedural -2 because it's a big hammer to essentially say "not right now". It overloads the meaning of the vote in a potentially confusing way that requires explanation every time it's used. At least I hope procedural -2's always include a comment.
This was exactly the reasoning. -2 is overloaded, but its primary meaning was/is "we do not want this code change". It just happens that it was also a convenient way to say that with "right now" at the end.
The Review-Priority -1 is a clear way to say whether something is held because it can't be merged right now due to procedural or process reasons, versus something that we just don't want at all. for what its worth my understanding of why a procdural -2 is more correct is that this change cannot be merged because it has not met the procedual requirement to be considerd for this release. haveing received several over the years i have never seen it to carry any malaise or weight then the zuul pep8 job complianing about the line lenght of my code. with either a procedural -2 or a verify -1 from zuul my code is equally un mergeable.
the prime example being a patch that requires a spec that has not been approved. while most cores will not approve chage when other cores have left a -1 mistakes happen and the -2 does emphasise the point that even if the code is perfect under the porject processes this change should not be acitvly reporposed until the issue raised by the -2 has been addressed. In the case of a procedual -2 that typically means the spec is merge or the master branch opens for the next cycle.
i agree that procedural -2's can seam harsh at first glance but i have also never seen one left without a comment explaining why it was left. the issue with a procedural -1 is i can jsut resubmit the patch several times and it can get lost in the comments.
I don't think that's a problem with this new field. It sounds like priority -1 carries over from PS to PS.
we recently intoduced a new review priority lable if we really wanted to disabiguate form normal -2s then we coudl have an explcitly lable for it but i personally would prefer to keep procedural -2s.
To be clear, I have both used and received procedural -2's as well and they don't particularly bother me, but I can see where if you were someone who was new to the community or just a part-time contributor not as familiar with our processes it might be an unpleasant experience to see that -2 show up. As I said, I don't know that I would advocate for this on the basis of replacing procedural -2 alone, but if we're adding the category anyway I mildly prefer using it for procedural blockers in the future.
On Fri, 2019-01-11 at 15:39 -0600, Ben Nemec wrote:
On 1/10/19 5:03 PM, Sean Mooney wrote:
On Thu, 2019-01-10 at 13:42 -0600, Sean McGinnis wrote:
I don't know if this was the reasoning behind Cinder's system, but I know some people object to procedural -2 because it's a big hammer to essentially say "not right now". It overloads the meaning of the vote in a potentially confusing way that requires explanation every time it's used. At least I hope procedural -2's always include a comment.
This was exactly the reasoning. -2 is overloaded, but its primary meaning was/is "we do not want this code change". It just happens that it was also a convenient way to say that with "right now" at the end.
The Review-Priority -1 is a clear way to say whether something is held because it can't be merged right now due to procedural or process reasons, versus something that we just don't want at all.
for what its worth my understanding of why a procdural -2 is more correct is that this change cannot be merged because it has not met the procedual requirement to be considerd for this release. haveing received several over the years i have never seen it to carry any malaise or weight then the zuul pep8 job complianing about the line lenght of my code. with either a procedural -2 or a verify -1 from zuul my code is equally un mergeable.
the prime example being a patch that requires a spec that has not been approved. while most cores will not approve chage when other cores have left a -1 mistakes happen and the -2 does emphasise the point that even if the code is perfect under the porject processes this change should not be acitvly reporposed until the issue raised by the -2 has been addressed. In the case of a procedual -2 that typically means the spec is merge or the master branch opens for the next cycle.
i agree that procedural -2's can seam harsh at first glance but i have also never seen one left without a comment explaining why it was left. the issue with a procedural -1 is i can jsut resubmit the patch several times and it can get lost in the comments.
I don't think that's a problem with this new field. It sounds like priority -1 carries over from PS to PS.
we recently intoduced a new review priority lable if we really wanted to disabiguate form normal -2s then we coudl have an explcitly lable for it but i personally would prefer to keep procedural -2s.
To be clear, I have both used and received procedural -2's as well and they don't particularly bother me, but I can see where if you were someone who was new to the community or just a part-time contributor not as familiar with our processes it might be an unpleasant experience to see that -2 show up. As I said, I don't know that I would advocate for this on the basis of replacing procedural -2 alone, but if we're adding the category anyway I mildly prefer using it for procedural blockers in the future.
i think i partally missunderstood the proposal. i had parsed it as replacing procedual code review -2 with a code review -1 rahter then review priority -1. if all projects adopt review priorty going forward then that might makes sense for those that dont i think code review -2 still makes sense.
This has worked great for Designate as most of the reviewers have limited time, it has helped us focus on core issues and get critical patches out a lot faster than we otherwise would. Sent from my iPhone
On Jan 10, 2019, at 9:14 AM, Ben Nemec <openstack@nemebean.com> wrote:
On Fri, Dec 28, 2018 at 11:04:41AM +0530, Surya Singh wrote:
Dear All,
There are many occasion when we want to priorities some of the patches whether it is related to unblock the gates or blocking the non freeze patches during RC.
So adding the Review-Priority will allow more precise dashboard. As Designate and Cinder projects already experiencing this[1][2] and after discussion with Jeremy brought this to ML to interact with these team before landing [3], as there is possibility that reapply the priority vote following any substantive updates to change could make it more cumbersome than it is worth.
With Cinder this is fairly new, but I think it is working well so far. The oddity we've run into, that I think you're referring to here, is how those votes carry forward with updates.
I set up Cinder with -1, +1, and +2 as possible priority votes. It appears when This idea looks great and helpful especially for blockers and cycle priority patches to get regular review bandwidth from Core or Active members of that project. IMO only +ve votes are more appropriate for this label. -1 is little confusing for many reasons like what is the difference between Review-Priority -1 and Code-Review -2 ? Review-Priority -1 means, it is less priority than 0/not labelled (explicitly setting any patch very less priority). After seeing Cinder dashboard, I got to know that -1 is used to block the changes due to procedural or technical reason. But that can be done by -2 on Code-Review label. Keeping Review-Priority label only for priority set makes it more clear which is nothing but allowing only +ve votes for this label. Personally, I prefer only a single vote set which can be +1 to convey that these are the set of changes
On 1/10/19 12:17 AM, Ghanshyam Mann wrote: ---- On Thu, 03 Jan 2019 22:51:55 +0900 Sean McGinnis <sean.mcginnis@gmx.com> wrote ---- priority for review but having multiple +ve vote set as per project need/interest is all fine.
I don't know if this was the reasoning behind Cinder's system, but I know some people object to procedural -2 because it's a big hammer to essentially say "not right now". It overloads the meaning of the vote in a potentially confusing way that requires explanation every time it's used. At least I hope procedural -2's always include a comment.
Whether adding a whole new vote type is a meaningful improvement is another question, but if we're adding the type anyway for prioritization it might make sense to use it to replace procedural -2. Especially if we could make it so any core can change it (apparently not right now), whereas -2 requires the original core to come back and remove it.
participants (12)
-
Ben Nemec
-
Clark Boylan
-
Doug Hellmann
-
Erik Olof Gunnar Andersson
-
Ghanshyam Mann
-
Jay Bryant
-
Jeremy Stanley
-
John Dickinson
-
Lance Bragstad
-
Sean McGinnis
-
Sean Mooney
-
Surya Singh