1189500 - Investigate if being able to have a limit for reviews per week would make sense

Status: RESOLVED WONTFIX

(bugzilla.mozilla.org :: Extensions, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

I was thinking if Bug 1003701 could be utilized in a bit automatic way, and also
in such way that the requester is informed why someone has reviews prevented.

So, one would have a review bucket per week - some pref telling how many reviews the reviewer
is willing to review each week, and once that limit is reached, 
bugzilla would automatically disable new review requests and tell to the requester that
the requestee has done x reviews already, and is unlikely to be able to do more before the next week.
Reviewer himself should always be able to add new requests to his queue, so that if 
someone kindly asks on IRC or so one to review yet another patch, reviewer could add
the patch to his own queue.

Comment 1

[:glob ✱]

this is a decision which the bugzilla team shouldn't own - we don't govern the review polices, they are decided by each team.

bsmedberg: i'd appreciate your feedback/thoughts.


my personal thoughts on this request is i don't think it will work.  the number of requests isn't an accurate measure of the time it will take to work through the review queue.  for example, if i set my limit to "10" and get 10 one-liner reviews on monday, then with your proposal i wouldn't be able to review any other patches for the remainder of the week.

i don't see how having a quota based review system would result in quicker review times.

Comment 2

[Ed Morley [:emorley]]

If I've understood correctly, it seems like the hope was that this feature:
* reduce the effort of reviewers using the "block requests" feature
* allow for informing the people making requests _why_ the user isn't accepting requests
* with the aim of helping distribute load more evenly between reviewers

Instead of this feature, I think more valuable would be:
* Making the suggested reviewer UI more discoverable and sorting the reviewer list in descending order of queue size (particularly for components where the number of suggested reviewers is high and the input field may overflow).
* Adding the ability to specify a reason (which could include the estimated timeframe of when the user is going to start accepting requests again), to help it become more socially acceptable to use the feature. (At the moment for people hitting the "this person isn't accepting requests" it isn't really clear why, and comes across as annoying).
* Making the existing feature more discoverable/known (though IMO it's pretty discoverable at present) - eg: adding a link to it from the bottom of the "Overdue request reminder" emails ("Overwhelmed with requests? Temporarily block new requests here: <url>"), or blogging about it.
* Adding the ability to specify a date when the requests blocking feature turns off - eg so people going on PTO don't have to remember to switch it off when they get back.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Ed Morley [:emorley] from comment #2)
> Instead of this feature, I think more valuable would be:
> * Making the suggested reviewer UI more discoverable and sorting the
> reviewer list in descending order of queue size
This doesn't help. As an example I don't usually have a long queue, but I review - well, a lot.

> * Adding the ability to specify a reason (which could include the estimated
> timeframe of when the user is going to start accepting requests again), to
> help it become more socially acceptable to use the feature. (At the moment
> for people hitting the "this person isn't accepting requests" it isn't
> really clear why, and comes across as annoying).
This sounds good indeed.

> * Making the existing feature more discoverable/known (though IMO it's
> pretty discoverable at present) - eg: adding a link to it from the bottom of
> the "Overdue request reminder" emails ("Overwhelmed with requests?
> Temporarily block new requests here: <url>")
sounds good.

> * Adding the ability to specify a date when the requests blocking feature
> turns off - eg so people going on PTO don't have to remember to switch it
> off when they get back.
yup.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

In addition to this, it _might_ be nice to have two queues. Review queue, which is limited, say to 30 reviews per week, and then overflowing review queue where review requests go if that limit of 30 is reached.
Though, it could be just one queue, where 'overflowing requests' are clearly marked and don't cause any warnings that one has X number of pending requests and the requester should also know when a request is "overflowing" (so that it is clear that reviewing time may be longer than normally).



(In reply to Byron Jones ‹:glob› from comment #1)
...
> i don't see how having a quota based review system would result in quicker
> review times.
That is not what the bug is about, not at all!
The bug is about limiting review requests for some particular reviewers, who are silly enough to do way too many reviews[1]. In other words, making the tools to force more evenly spread review load.




[1] I'm having hard time to achieve my Q3 goal to review less ;)

Comment 5

[Ed Morley [:emorley]]

(In reply to Ed Morley [:emorley] from comment #2)
> * Adding the ability to specify a reason (which could include the estimated
> timeframe of when the user is going to start accepting requests again)

Filed bug 1205656.

> * Making the existing feature more discoverable/known (though IMO it's
> pretty discoverable at present) - eg: adding a link to it from the bottom of
> the "Overdue request reminder" emails ("Overwhelmed with requests?
> Temporarily block new requests here: <url>")

Filed bug 1205663.

> * Adding the ability to specify a date when the requests blocking feature
> turns off - eg so people going on PTO don't have to remember to switch it
> off when they get back.

Filed bug 1205659.

Comment 6

[Benjamin Smedberg]

I am philosophically opposed to this change, in that reviews should always take priority and it is a terrible idea for module owners/peers to stop accepting review requests, either automatically or manually. I don't think I'm the right person to actually WONTFIX this bug, though.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Reviews always taking priority may lead to high reviewing overload and I think doing say 40+ reviews per week is more than enough. (Luckily I've had now 2 weeks already in row when there has been less than 40 reviews)

Comment 8

[:glob ✱]

as the owner of bmo i'll make the call and wontfix this bug.

setting a hard per-review limit is near impossible to get right, as it doesn't account for review size nor does it mesh well with the move towards breaking changes into small multiple patches.

if you're assigned a review that you're unable to do (for whatever reason) it's trivial to reassign the review to someone else.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Byron Jones ‹:glob› from comment #8)
> if you're assigned a review that you're unable to do (for whatever reason)
> it's trivial to reassign the review to someone else.

I disagree with this a bit. If I need to reassign a review request, I need to first wonder who else could review the patch, and then check if his/her review queue is long and then try reassigning
(and lately it has happened couple of times that the person I tried to reassign the request to wasn't accepting any review requests).
It really should be patch author's job, by default, to find a proper reviewer, and tools could hint about the possible good reviewers.

Comment 10

[David Lawrence [:dkl]]

(In reply to Olli Pettay [:smaug] from comment #9)
> I disagree with this a bit. If I need to reassign a review request, I need
> to first wonder who else could review the patch, and then check if his/her
> review queue is long and then try reassigning
> (and lately it has happened couple of times that the person I tried to
> reassign the request to wasn't accepting any review requests).
> It really should be patch author's job, by default, to find a proper
> reviewer, and tools could hint about the possible good reviewers.

Most of the time, you can click on the "suggested reviewers" link next to the review flag which will show a list of possibilities as well as the number of reviews each person currently has. So it does help some in this regard.

dkl

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I should have been more exact what I wrote. Since the current number of review requests isn't so interesting. More interesting is how many reviews someone has done recently - assuming we want to even out reviewing a bit.

Comment 12

[Andrew Overholt [:overholt]]

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #11)
> More interesting is how many reviews
> someone has done recently - assuming we want to even out reviewing a bit.

Should we reopen this requesting that specifically?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Or I could file a new bug for that. Let me do that in a bit (after a review ;) ).