Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Flavio Percoco]

On 07/10/15 10:17 -0400, Doug Hellmann wrote:

Excerpts from Flavio Percoco's message of 2015-10-07 16:50:16 +0900:

On 06/10/15 23:36 +0900, Flavio Percoco wrote:
>Greetings,
>
>Not so long ago, Erno started a thread[0] in this list to discuss the
>abandon policies for patches that haven't been updated in Glance.
>
>I'd like to go forward and start following that policy with some
>changes that you can find below:
>
>1) Lets do this on patches that haven't had any activity in the last 2
>months. This adds one more month to Erno's proposal. The reason being
>that during the lat cycle, there were some ups and downs in the review
>flow that caused some patches to get stuck.
>
>2) Do this just on master, for all patches regardless they fix a
>bug or implement a spec and for all patches regardless their review
>status.
>
>3) The patch will be first marked as a WIP and then abandoned if the
>patch is not updated in 1 week. This will put this patches at the
>begining of the queue but using the Glance review dashboard should
>help keeing focus.
>
>Unless there are some critical things missing in the above or strong
>opiniones against this, I'll make this effective starting next Monday
>October 12th.

I'd like to provide some extra data here. This is our current status:

==
Total patches without activity in the last 2 months: 73
Total patches closing a bug: 30
Total patches with negative review by core reviewers: 62
Total patches with negative review by non-core reviewers: 75
Total patches without a core review in the last patchset: 13
Total patches with negative review from Jenkins: 50
==

It's not ideal but it's also not a lot. I'd like to recover as many
patches as possible from the above and I'm happy to do that manually
if necessary.

Cheers,
Flavio



It might be useful to schedule a review sprint to take a couple of days
to focus on reviews. Maybe the team can pick dates a couple of weeks in
advance so everyone can get permission to spend the full time on
reviews.


+1 This is something I've planned.

Flavio

--
@flaper87
Flavio Percoco


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Flavio Percoco]

On 06/10/15 12:11 -0400, Nikhil Komawar wrote:

Overall I think this is a good idea and the time frame proposal also looks
good. Few suggestions in-line.

On 10/6/15 10:36 AM, Flavio Percoco wrote:

   Greetings,

   Not so long ago, Erno started a thread[0] in this list to discuss the
   abandon policies for patches that haven't been updated in Glance.

   I'd like to go forward and start following that policy with some
   changes that you can find below:

   1) Lets do this on patches that haven't had any activity in the last 2
   months. This adds one more month to Erno's proposal. The reason being
   that during the lat cycle, there were some ups and downs in the review
   flow that caused some patches to get stuck.



+2 . I think 2 months is a reasonable time frame. Though, I think this should
be done on glance , python-glanceclient and glance-store repos and not
glance-specs. Specs can sometimes need to sit and wait while discussion may
happen at other places and then a gist is added back the spec.


Yup, no plans to apply this to glance-specs, just code.

Thanks for the feedback,
Flavio




   2) Do this just on master, for all patches regardless they fix a
   bug or implement a spec and for all patches regardless their review
   status.



+2 . No comments, looks clean.


   3) The patch will be first marked as a WIP and then abandoned if the
   patch is not updated in 1 week. This will put this patches at the
   begining of the queue but using the Glance review dashboard should
   help keeing focus.



While I think that one may give someone a email/irc heads up if the proposer
doesn't show up and we will use the context and wisdom of feedback this sorta
seems to imply for a general case when a developer is new and their intent to
get a patch in one cycle isn't clear.


   Unless there are some critical things missing in the above or strong
   opiniones against this, I'll make this effective starting next Monday
   October 12th.



I added some comments above for possible brainstorming. No serious objections,
looking forward to this cleanup process.


   Best regards,
   Flavio

   [0] http://lists.openstack.org/pipermail/openstack-dev/2015-February/
   056829.html



  


   __
   OpenStack Development Mailing List (not for usage questions)
   Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
   http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


--

Thanks,
Nikhil



--
@flaper87
Flavio Percoco


pgpK_pi2Tj7Yr.pgp
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Flavio Percoco]

On 06/10/15 17:54 +0200, Victor Stinner wrote:

Hi,

Le 06/10/2015 16:36, Flavio Percoco a écrit :

Not so long ago, Erno started a thread[0] in this list to discuss the
abandon policies for patches that haven't been updated in Glance.
(...)
1) Lets do this on patches that haven't had any activity in the last 2
months. This adds one more month to Erno's proposal. The reason being
that during the lat cycle, there were some ups and downs in the review
flow that caused some patches to get stuck.


Please don't do that. I sent a patch in June (20) and it was only 
reviewed in October (4)... There was no activity simply because I had 
nothing to add, everything was explained in the commit message, I was 
only waiting for a review...


I came on #openstack-glance to ask for review several time between 
August and September but nobody reviewed by patches (there was al.


Example of patch: https://review.openstack.org/#/c/193786/ (now merged)


Yes, I'm very aware of this case and it's great feedback. What
happened with these patches could have (or did happen) with other
patches. The review response is something that I'd definitely like us
to improve and this is not the solution.



It would be very frustrating to have to resend the same patch over and over.


There's no need to resend the patch. Just commenting saying that it's
still a valid update and it'll keep the patch from being abandoned.

Cheers,
Flavio

--
@flaper87
Flavio Percoco


pgpmF99M4Fd9v.pgp
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Flavio Percoco]

On 06/10/15 23:36 +0900, Flavio Percoco wrote:

Greetings,

Not so long ago, Erno started a thread[0] in this list to discuss the
abandon policies for patches that haven't been updated in Glance.

I'd like to go forward and start following that policy with some
changes that you can find below:

1) Lets do this on patches that haven't had any activity in the last 2
months. This adds one more month to Erno's proposal. The reason being
that during the lat cycle, there were some ups and downs in the review
flow that caused some patches to get stuck.

2) Do this just on master, for all patches regardless they fix a
bug or implement a spec and for all patches regardless their review
status.

3) The patch will be first marked as a WIP and then abandoned if the
patch is not updated in 1 week. This will put this patches at the
begining of the queue but using the Glance review dashboard should
help keeing focus.

Unless there are some critical things missing in the above or strong
opiniones against this, I'll make this effective starting next Monday
October 12th.


I'd like to provide some extra data here. This is our current status:

==
Total patches without activity in the last 2 months: 73
Total patches closing a bug: 30
Total patches with negative review by core reviewers: 62
Total patches with negative review by non-core reviewers: 75
Total patches without a core review in the last patchset: 13
Total patches with negative review from Jenkins: 50
==

It's not ideal but it's also not a lot. I'd like to recover as many
patches as possible from the above and I'm happy to do that manually
if necessary.

Cheers,
Flavio


--
@flaper87
Flavio Percoco


pgpEt3dCWEcon.pgp
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Flavio Percoco]

On 06/10/15 17:52 +0200, Julien Danjou wrote:

On Tue, Oct 06 2015, Flavio Percoco wrote:

I send patches to Glance from time to time, and they usually got 0
review for *weeks* (sometimes months, because, well there are no
reviewers active in Glance, so:


1) Lets do this on patches that haven't had any activity in the last 2
months. This adds one more month to Erno's proposal. The reason being
that during the lat cycle, there were some ups and downs in the review
flow that caused some patches to get stuck.


This is going to expire my patches that nobody cares about and that are
improving the code or fixing stuff people didn't encounter (yet).


3) The patch will be first marked as a WIP and then abandoned if the
patch is not updated in 1 week. This will put this patches at the
begining of the queue but using the Glance review dashboard should
help keeing focus.


Why WIP? If a patch is complete and waiting for reviewers I'm not sure
it helps.


WIP because I don't think we should abandon them right away - since
there are patches like yours and Victor's that matter - and there's no
status to say: "I'm sorry we screwed up and we didn't review your
patch. Please come to us and throw all your amazing patches in our
faces so that we'll review them... for realz"


The problem is that nobody is reviewing Glance patches (except you
recently it seems). That's not going to solve that. That's just going to
hide the issues under the carpet by lowering the total of patches that
needs review…


I'm not trying to solve the lack of reviews in Liberty by removing
patches. What I'd like to do, though, is help to keep around patches
that really matter.

I know there have been a huge lag on reviews which is something that
we'll be working on with a different workflow. The dashboard mentioned
is one of them.

We could certainly increase the number of months.

Thanks a lot for the feedback,
Flavio




My 2c,

--
Julien Danjou
;; Free Software hacker
;; https://julien.danjou.info




--
@flaper87
Flavio Percoco


pgpDx8liLHnv6.pgp
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Flavio Percoco]

On 06/10/15 13:53 -0400, Doug Hellmann wrote:

Excerpts from Flavio Percoco's message of 2015-10-06 23:36:53 +0900:

Greetings,

Not so long ago, Erno started a thread[0] in this list to discuss the
abandon policies for patches that haven't been updated in Glance.

I'd like to go forward and start following that policy with some
changes that you can find below:

1) Lets do this on patches that haven't had any activity in the last 2
months. This adds one more month to Erno's proposal. The reason being
that during the lat cycle, there were some ups and downs in the review
flow that caused some patches to get stuck.

2) Do this just on master, for all patches regardless they fix a
bug or implement a spec and for all patches regardless their review
status.

3) The patch will be first marked as a WIP and then abandoned if the
patch is not updated in 1 week. This will put this patches at the
begining of the queue but using the Glance review dashboard should
help keeing focus.

Unless there are some critical things missing in the above or strong
opiniones against this, I'll make this effective starting next Monday
October 12th.

Best regards,
Flavio

[0] http://lists.openstack.org/pipermail/openstack-dev/2015-February/056829.html



In the past we've had discussions on the list about how abandoning
patches can be perceived as hostile to contributors, and that using
a review dashboard with good filters is a better solution. Since
you already have a dashboard, I suggest adding a section for patches
that are old but have no review comments (maybe you already have
that) and another for patches where the current viewer has voted
-1. The first highlights the patches for reviewers, and ignores
them when they are in a state where we're waiting for feedback or
an update, and the latter provides a list of patches the current
reviewer is involved in and may need to recheck for new comments.


We definitely don't want anyone to feel bad about this, especially
when it's our fault they patches are still hanging around. This is the
reason why the WIP phase is being proposed as away to ask the user to
come to use and remind us to do our job. Not ideal, sure, but it
happens to everyone.

The current dashboard has that section already and I really hope we
don't get to the point where abandoning patches ourselves is
necessary. The section is called "5 Days Without Feedback" and I gotta
be honest, it's worked (the dashboard in general) very well for me and
I hope for others as well.

There are patches from old contributors that we just know are never
going to be worked out. These patches I'd like to take a look closely
before abandoning them so that we can rebase them ourselves if they
are still relevant. Making this list shorter is what I'd like to
achieve with the proposed plan.

Hope the above makes sense,
Flavio

--
@flaper87
Flavio Percoco


pgpSJOU3hsy5w.pgp
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Flavio Percoco]

On 07/10/15 11:12 +0200, Julien Danjou wrote:

On Wed, Oct 07 2015, Flavio Percoco wrote:


I'm not trying to solve the lack of reviews in Liberty by removing
patches. What I'd like to do, though, is help to keep around patches
that really matter.


I think that's where you are making a mistake. They are contributors,
like me or Victor, are knocking on the Glance doors for months now,
sending patches that resolve technical debt rather than adding new
debt^Wfeatures. Currently, these patches are not seen as important and
are often "dismissed". So I'm pretty sure they are going to expire with
this new system.


I can't do anything for the past failures other than saying I'm sorry.
As I mentioned in previous emails in this thread, the work to make the
review process better is unrelated to the topic on this email, really.

I wouldn't say that your patches (or Victor's) weren't important for
the team but I would like to avoid getting into the details of the
past, tbh.


Imagine that if you were merging patches from me, Victor, and people
like us, we would continue to send many of them, and mid-term, you'd get
some new blood on your core team.


I don't think this needs to be explained and I trust the whole Glance
core team to know this. Although, it's better to be explicit than
implicit so, thanks.



What is proposed here is really focusing on making life easier for the
current core team which is in large majority inactive.


This is were I think I'm failing to communicate the intention here.
The dashboard[0] I've put up is the one that intends to make the core
team's life easier.

[0] http://bit.ly/glance-review-dashboard



Don't read me wrong. I know you and Nikhil are both well-intentioned by
proposing that. I just think it's going to be worse, because it won't
improve much and you're going to push new contributors away.


Absolutely, I value everyone's feedback on this thread a lot. I hope
I'm explaining the goal correctly. If I'm not, I'm more than happy to
talk more about this (See my email with some stats for example).

Cheers,
Flavio


--
@flaper87
Flavio Percoco


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Julien Danjou]

On Wed, Oct 07 2015, Flavio Percoco wrote:

> I'm not trying to solve the lack of reviews in Liberty by removing
> patches. What I'd like to do, though, is help to keep around patches
> that really matter.

I think that's where you are making a mistake. They are contributors,
like me or Victor, are knocking on the Glance doors for months now,
sending patches that resolve technical debt rather than adding new
debt^Wfeatures. Currently, these patches are not seen as important and
are often "dismissed". So I'm pretty sure they are going to expire with
this new system.

Imagine that if you were merging patches from me, Victor, and people
like us, we would continue to send many of them, and mid-term, you'd get
some new blood on your core team.

What is proposed here is really focusing on making life easier for the
current core team which is in large majority inactive.

Don't read me wrong. I know you and Nikhil are both well-intentioned by
proposing that. I just think it's going to be worse, because it won't
improve much and you're going to push new contributors away.

-- 
Julien Danjou
# Free Software hacker
# https://julien.danjou.info


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Bunting, Niall]

> From: Julien Danjou [jul...@danjou.info]
> Sent: 07 October 2015 10:12
> 
> On Wed, Oct 07 2015, Flavio Percoco wrote:
> 
> > I'm not trying to solve the lack of reviews in Liberty by removing
> > patches. What I'd like to do, though, is help to keep around patches
> > that really matter.
> 
> I think that's where you are making a mistake. They are contributors,
> like me or Victor, are knocking on the Glance doors for months now,
> sending patches that resolve technical debt rather than adding new
> debt^Wfeatures. Currently, these patches are not seen as important and
> are often dismissed. So I'm pretty sure they are going to expire with
> this new system.
> 
> Imagine that if you were merging patches from me, Victor, and people
> like us, we would continue to send many of them, and mid-term, you'd get
> some new blood on your core team.
> 
> What is proposed here is really focusing on making life easier for the
> current core team which is in large majority inactive.
> 
> Don't read me wrong. I know you and Nikhil are both well-intentioned by
> proposing that. I just think it's going to be worse, because it won't
> improve much and you're going to push new contributors away.

If your patches are sitting there waiting for review once they get off
the top couple of pages they are likely to become buried, as they are
waiting for review. This is unlikely to benefit anyone.

How would an active user keep their patches it the active/up for review
state? By just posting bump comments?

Any type of change Juilen is talking about could keep on being dismissed,
and this could end up in some sort of game to keep your patch above
the non-review line for it just to be ignored. This would definitely be
a bad thing, therefore I think any patches that are picked up as old,
should be reviewed before being marked as WIP. As this would mean if
the patch is moved out of WIP it would be less likely to get stuck in
some sort of loop as it has a review.

We also have to be careful about alienating contributors, we should make
sure they know why there work got marked WIP with a link to a wiki page
explaining the process. However if this forces a review, they may also
be happy that they eventually got a review on their patch.

My thoughts,
Niall

Edit: As this did not send to the list originally.
Flavio points out that they aim to review patches that are unmarked wip,
I think that system could work as long as it avoids the problem of patches
potentially becoming stuck in a circle.

And It should be kept in mind that even old reviews could still be relevant,
and the best course of action may not to just mark them wip without thought.
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Flavio Percoco]

On 07/10/15 12:47 +, Bunting, Niall wrote:

From: Julien Danjou [jul...@danjou.info]
Sent: 07 October 2015 10:12

On Wed, Oct 07 2015, Flavio Percoco wrote:

> I'm not trying to solve the lack of reviews in Liberty by removing
> patches. What I'd like to do, though, is help to keep around patches
> that really matter.

I think that's where you are making a mistake. They are contributors,
like me or Victor, are knocking on the Glance doors for months now,
sending patches that resolve technical debt rather than adding new
debt^Wfeatures. Currently, these patches are not seen as important and
are often dismissed. So I'm pretty sure they are going to expire with
this new system.

Imagine that if you were merging patches from me, Victor, and people
like us, we would continue to send many of them, and mid-term, you'd get
some new blood on your core team.

What is proposed here is really focusing on making life easier for the
current core team which is in large majority inactive.

Don't read me wrong. I know you and Nikhil are both well-intentioned by
proposing that. I just think it's going to be worse, because it won't
improve much and you're going to push new contributors away.


If your patches are sitting there waiting for review once they get off
the top couple of pages they are likely to become buried, as they are
waiting for review. This is unlikely to benefit anyone.

How would an active user keep their patches it the active/up for review
state? By just posting bump comments?


No one should need to keep their reviews in the "active" queue as
there's not really an active queue. There are just tons of pages.

When a patch is marked as WIP because it hasn't been updated, the user
just needs to comment on it to keep it around. The WIP flag will be
removed then. The comment is needed because one cannot remove the WIP
flag of other users in gerrit.

If there are un-addressed comments, proposing a patch-set addressing
these comments would be the best/right way to remove the WIP flag.

Also, the WIP is not really mandatory. We could do away with it. I
think it'd be useful for users as it'd be clear that something is
happening in their patches when they load their personal dashboard.
Many folks ignore gerrit emails and they may miss the comment update,
which is what really worries me.


Any type of change Juilen is talking about could keep on being dismissed,
and this could end up in some sort of game to keep your patch above
the non-review line for it just to be ignored. This would definitely be
a bad thing, therefore I think any patches that are picked up as old,
should be reviewed before being marked as WIP. As this would mean if
the patch is moved out of WIP it would be less likely to get stuck in
some sort of loop as it has a review.

We also have to be careful about alienating contributors, we should make
sure they know why there work got marked WIP with a link to a wiki page
explaining the process. However if this forces a review, they may also
be happy that they eventually got a review on their patch.


The plan is not to just mark as WIP and be done with it. The plan is
to obviously provide enough information for users to know what the
next step is. I should've probably mentioned this in my original
email.



My thoughts,
Niall

Edit: As this did not send to the list originally.
Flavio points out that they aim to review patches that are unmarked wip,
I think that system could work as long as it avoids the problem of patches
potentially becoming stuck in a circle.

And It should be kept in mind that even old reviews could still be relevant,
and the best course of action may not to just mark them wip without thought.


Again, the plan is not to just go and blindly abandon. The above is
important and certainly being kept in mind.

Flavio

--
@flaper87
Flavio Percoco


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Doug Hellmann]

Excerpts from Flavio Percoco's message of 2015-10-07 16:50:16 +0900:
> On 06/10/15 23:36 +0900, Flavio Percoco wrote:
> >Greetings,
> >
> >Not so long ago, Erno started a thread[0] in this list to discuss the
> >abandon policies for patches that haven't been updated in Glance.
> >
> >I'd like to go forward and start following that policy with some
> >changes that you can find below:
> >
> >1) Lets do this on patches that haven't had any activity in the last 2
> >months. This adds one more month to Erno's proposal. The reason being
> >that during the lat cycle, there were some ups and downs in the review
> >flow that caused some patches to get stuck.
> >
> >2) Do this just on master, for all patches regardless they fix a
> >bug or implement a spec and for all patches regardless their review
> >status.
> >
> >3) The patch will be first marked as a WIP and then abandoned if the
> >patch is not updated in 1 week. This will put this patches at the
> >begining of the queue but using the Glance review dashboard should
> >help keeing focus.
> >
> >Unless there are some critical things missing in the above or strong
> >opiniones against this, I'll make this effective starting next Monday
> >October 12th.
> 
> I'd like to provide some extra data here. This is our current status:
> 
> ==
> Total patches without activity in the last 2 months: 73
> Total patches closing a bug: 30
> Total patches with negative review by core reviewers: 62
> Total patches with negative review by non-core reviewers: 75
> Total patches without a core review in the last patchset: 13
> Total patches with negative review from Jenkins: 50
> ==
> 
> It's not ideal but it's also not a lot. I'd like to recover as many
> patches as possible from the above and I'm happy to do that manually
> if necessary.
> 
> Cheers,
> Flavio
> 

It might be useful to schedule a review sprint to take a couple of days
to focus on reviews. Maybe the team can pick dates a couple of weeks in
advance so everyone can get permission to spend the full time on
reviews.

Doug

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Doug Hellmann]

Excerpts from Flavio Percoco's message of 2015-10-06 23:36:53 +0900:
> Greetings,
> 
> Not so long ago, Erno started a thread[0] in this list to discuss the
> abandon policies for patches that haven't been updated in Glance.
> 
> I'd like to go forward and start following that policy with some
> changes that you can find below:
> 
> 1) Lets do this on patches that haven't had any activity in the last 2
> months. This adds one more month to Erno's proposal. The reason being
> that during the lat cycle, there were some ups and downs in the review
> flow that caused some patches to get stuck.
> 
> 2) Do this just on master, for all patches regardless they fix a
> bug or implement a spec and for all patches regardless their review
> status.
> 
> 3) The patch will be first marked as a WIP and then abandoned if the
> patch is not updated in 1 week. This will put this patches at the
> begining of the queue but using the Glance review dashboard should
> help keeing focus.
> 
> Unless there are some critical things missing in the above or strong
> opiniones against this, I'll make this effective starting next Monday
> October 12th.
> 
> Best regards,
> Flavio
> 
> [0] 
> http://lists.openstack.org/pipermail/openstack-dev/2015-February/056829.html
> 

In the past we've had discussions on the list about how abandoning
patches can be perceived as hostile to contributors, and that using
a review dashboard with good filters is a better solution. Since
you already have a dashboard, I suggest adding a section for patches
that are old but have no review comments (maybe you already have
that) and another for patches where the current viewer has voted
-1. The first highlights the patches for reviewers, and ignores
them when they are in a state where we're waiting for feedback or
an update, and the latter provides a list of patches the current
reviewer is involved in and may need to recheck for new comments.

Doug

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Nikhil Komawar]

On 10/6/15 1:53 PM, Doug Hellmann wrote:
> Excerpts from Flavio Percoco's message of 2015-10-06 23:36:53 +0900:
>> Greetings,
>>
>> Not so long ago, Erno started a thread[0] in this list to discuss the
>> abandon policies for patches that haven't been updated in Glance.
>>
>> I'd like to go forward and start following that policy with some
>> changes that you can find below:
>>
>> 1) Lets do this on patches that haven't had any activity in the last 2
>> months. This adds one more month to Erno's proposal. The reason being
>> that during the lat cycle, there were some ups and downs in the review
>> flow that caused some patches to get stuck.
>>
>> 2) Do this just on master, for all patches regardless they fix a
>> bug or implement a spec and for all patches regardless their review
>> status.
>>
>> 3) The patch will be first marked as a WIP and then abandoned if the
>> patch is not updated in 1 week. This will put this patches at the
>> begining of the queue but using the Glance review dashboard should
>> help keeing focus.
>>
>> Unless there are some critical things missing in the above or strong
>> opiniones against this, I'll make this effective starting next Monday
>> October 12th.
>>
>> Best regards,
>> Flavio
>>
>> [0] 
>> http://lists.openstack.org/pipermail/openstack-dev/2015-February/056829.html
>>
> In the past we've had discussions on the list about how abandoning
> patches can be perceived as hostile to contributors, and that using
> a review dashboard with good filters is a better solution. Since
> you already have a dashboard, I suggest adding a section for patches
> that are old but have no review comments (maybe you already have
> that) and another for patches where the current viewer has voted
> -1. The first highlights the patches for reviewers, and ignores
> them when they are in a state where we're waiting for feedback or
> an update, and the latter provides a list of patches the current
> reviewer is involved in and may need to recheck for new comments.

That hostility aspect is the main reason why abandoning has been avoided
until now.

> Doug
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-- 

Thanks,
Nikhil


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Julien Danjou]

On Tue, Oct 06 2015, Flavio Percoco wrote:

I send patches to Glance from time to time, and they usually got 0
review for *weeks* (sometimes months, because, well there are no
reviewers active in Glance, so:

> 1) Lets do this on patches that haven't had any activity in the last 2
> months. This adds one more month to Erno's proposal. The reason being
> that during the lat cycle, there were some ups and downs in the review
> flow that caused some patches to get stuck.

This is going to expire my patches that nobody cares about and that are
improving the code or fixing stuff people didn't encounter (yet).

> 3) The patch will be first marked as a WIP and then abandoned if the
> patch is not updated in 1 week. This will put this patches at the
> begining of the queue but using the Glance review dashboard should
> help keeing focus.

Why WIP? If a patch is complete and waiting for reviewers I'm not sure
it helps.

The problem is that nobody is reviewing Glance patches (except you
recently it seems). That's not going to solve that. That's just going to
hide the issues under the carpet by lowering the total of patches that
needs review…

My 2c,

-- 
Julien Danjou
;; Free Software hacker
;; https://julien.danjou.info


signature.asc
Description: PGP signature
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Victor Stinner]

Hi,

Le 06/10/2015 16:36, Flavio Percoco a écrit :

Not so long ago, Erno started a thread[0] in this list to discuss the
abandon policies for patches that haven't been updated in Glance.
(...)
1) Lets do this on patches that haven't had any activity in the last 2
months. This adds one more month to Erno's proposal. The reason being
that during the lat cycle, there were some ups and downs in the review
flow that caused some patches to get stuck.


Please don't do that. I sent a patch in June (20) and it was only 
reviewed in October (4)... There was no activity simply because I had 
nothing to add, everything was explained in the commit message, I was 
only waiting for a review...


I came on #openstack-glance to ask for review several time between 
August and September but nobody reviewed by patches (there was al.


Example of patch: https://review.openstack.org/#/c/193786/ (now merged)

It would be very frustrating to have to resend the same patch over and over.

Victor

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Nikhil Komawar]

Overall I think this is a good idea and the time frame proposal also
looks good. Few suggestions in-line.

On 10/6/15 10:36 AM, Flavio Percoco wrote:
> Greetings,
>
> Not so long ago, Erno started a thread[0] in this list to discuss the
> abandon policies for patches that haven't been updated in Glance.
>
> I'd like to go forward and start following that policy with some
> changes that you can find below:
>
> 1) Lets do this on patches that haven't had any activity in the last 2
> months. This adds one more month to Erno's proposal. The reason being
> that during the lat cycle, there were some ups and downs in the review
> flow that caused some patches to get stuck.
>

+2 . I think 2 months is a reasonable time frame. Though, I think this
should be done on glance , python-glanceclient and glance-store repos
and not glance-specs. Specs can sometimes need to sit and wait while
discussion may happen at other places and then a gist is added back the
spec.

> 2) Do this just on master, for all patches regardless they fix a
> bug or implement a spec and for all patches regardless their review
> status.
>

+2 . No comments, looks clean.

> 3) The patch will be first marked as a WIP and then abandoned if the
> patch is not updated in 1 week. This will put this patches at the
> begining of the queue but using the Glance review dashboard should
> help keeing focus.
>

While I think that one may give someone a email/irc heads up if the
proposer doesn't show up and we will use the context and wisdom of
feedback this sorta seems to imply for a general case when a developer
is new and their intent to get a patch in one cycle isn't clear.

> Unless there are some critical things missing in the above or strong
> opiniones against this, I'll make this effective starting next Monday
> October 12th.
>

I added some comments above for possible brainstorming. No serious
objections, looking forward to this cleanup process.

> Best regards,
> Flavio
>
> [0]
> http://lists.openstack.org/pipermail/openstack-dev/2015-February/056829.html
>
>
>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-- 

Thanks,
Nikhil

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Nikhil Komawar]

This again becomes a part of the undefinied priority concept. It's hard
to come up with a list of priorities all at the beginning of the cycle
so, conflicting priority items may get missed every now and then. I hope
the dashboard will help however, this is more of a people problem than a
process problem what Victor is describing.

To solve that, here's the context:
If a change is either big, has a spec associated with it or doesn't have
a bug associated it is unlikely to catch the notice of the cores easily
let alone other reviewers. Something that would help is to propose a
topic to the weekly meeting and if you can't attend delegate it to
someone who will -- I have seen it help catch attention of the cores.
Specs and other big changes are often discussed at the drivers meeting
so that's a good place to bring this up too. It doesn't have to be a
long one, just a heads up suffices many a times.

On 10/6/15 11:54 AM, Victor Stinner wrote:
> Hi,
>
> Le 06/10/2015 16:36, Flavio Percoco a écrit :
>> Not so long ago, Erno started a thread[0] in this list to discuss the
>> abandon policies for patches that haven't been updated in Glance.
>> (...)
>> 1) Lets do this on patches that haven't had any activity in the last 2
>> months. This adds one more month to Erno's proposal. The reason being
>> that during the lat cycle, there were some ups and downs in the review
>> flow that caused some patches to get stuck.
>
> Please don't do that. I sent a patch in June (20) and it was only
> reviewed in October (4)... There was no activity simply because I had
> nothing to add, everything was explained in the commit message, I was
> only waiting for a review...
>
> I came on #openstack-glance to ask for review several time between
> August and September but nobody reviewed by patches (there was al.
>
> Example of patch: https://review.openstack.org/#/c/193786/ (now merged)
>
> It would be very frustrating to have to resend the same patch over and
> over.
>
> Victor
>
> __
>
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe:
> openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-- 

Thanks,
Nikhil


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [Glance] Process to clean up the review queue from non-active patches

[Nikhil Komawar]

I think Glance reviewer brandwidth is pretty low and the feedback time
can be quite high. For specs, I had requested a section describing the
core reviewer who will be vouching for your spec to be added to the spec
itself. I think in general we have not seen anyone doing that strongly.

If a spec isn't proposed as a priority at the beginning of a summit and
if it's a big change and is not decided to an extent, it becomes quite
tricky to get in the project in that cycle. As a general feedback I have
always recommend bringing feature proposals at summit sessions,
mid-cycle, meetings etc etc. If a feature has missed it other proposal
have gotten priority. Also, there were priorities that were set the
beginning of the cycle and if a proposal isn't on the list there, it
becomes tricky to make a call for a new spec to get merged in one cycle.



Happy to help on the process, review speed, activities 'context' more if
you want input.

On 10/6/15 11:52 AM, Julien Danjou wrote:
> On Tue, Oct 06 2015, Flavio Percoco wrote:
>
> I send patches to Glance from time to time, and they usually got 0
> review for *weeks* (sometimes months, because, well there are no
> reviewers active in Glance, so:
>
>> 1) Lets do this on patches that haven't had any activity in the last 2
>> months. This adds one more month to Erno's proposal. The reason being
>> that during the lat cycle, there were some ups and downs in the review
>> flow that caused some patches to get stuck.
> This is going to expire my patches that nobody cares about and that are
> improving the code or fixing stuff people didn't encounter (yet).
>
>> 3) The patch will be first marked as a WIP and then abandoned if the
>> patch is not updated in 1 week. This will put this patches at the
>> begining of the queue but using the Glance review dashboard should
>> help keeing focus.
> Why WIP? If a patch is complete and waiting for reviewers I'm not sure
> it helps.
>
> The problem is that nobody is reviewing Glance patches (except you
> recently it seems). That's not going to solve that. That's just going to
> hide the issues under the carpet by lowering the total of patches that
> needs review…
>
> My 2c,
>
>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-- 

Thanks,
Nikhil

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev