[squid-dev] Timeouts for abandoned negative reviews

[Amos Jeffries]

On 9/01/20 11:20 am, Alex Rousskov wrote:
> Hello,
> 
>     Squid GitHub pull requests have the following problem: A core
> developer can stall PR progress by submitting a negative review and then
> ignoring the PR (despite others reminding them that the reviewer action
> is required). Such stalled PRs cannot be merged because our policies
> strictly prohibit merging of vetoed PRs.

The "problem" you are describing is what I see as a core principle of
the review process.

Consider the case of Joe blogs coming along tomorrow to submits a PR
deleting all of ICAP support from Squid.
 day 1: one reviewer gives it a no-go vote.
 day N: Auto-Merge? oops.


> 
> This problem has affected many PRs.

This situation is not new. I have 400-800 patch submissions from before
the github days that got blocked for various reasons and need polishing
up to re-submit.

One of the oldest is a still-active patch that was submitted in Oct 2010
for improved talloc support. It is being used by some clients from that
period and blocked by reviewer who did not like how the author could not
definitively prove that it would work with all compilers on every
operating system.


> Collection of meaningful stats is
> prohibitively expensive, but there are ~50 PRs that were open for 100+
> days and many of them are not abandoned/irrelevant. Here are some of the
> worst examples (the stalled day counts below are approximate):
> 
> * PR 369: stalled for 310 days (and counting)
>   https://github.com/squid-cache/squid/pull/369
> 
> * PR 59: 120-480 days, depending on when you think the PR was stalled
>   https://github.com/squid-cache/squid/pull/59
> 
> * PR 443: stalled for 100+ days (and counting)
>   https://github.com/squid-cache/squid/pull/443
> 

* PR 30 blocked because the reviewer wants author to fix bugs in
pre-existing code.

(IMO the 'days' metric is less indicative than the PR number itself.)


Also, at a fundamental level I object to the categorization of any open
PRs as "abandoned".

There is a lot of maintainer work in the background which nobody ever sees.

Particular to this proposal I regularly review *all* PRs in a quick scan
to see where progress can happen, as I did for the patches queue under
pre-github systems. What gets omitted is a post to every PR saying "I
looked at this today - decided it was too much work for the next {1,2,3}
hours I have available".

 [ By regular I mean at least once a week. I have two regular days with
usually 3-4 hrs free (you will see most larger audits happen these
days). And 3-4 whole days in a month (you might see _one_ of the larger
PRs worked on each of those days, or it may be worked on but not pushed
yet). ]



> Stalled PRs may significantly increase development overheads and badly
> reflect on the Squid Project as a whole. I have heard many complains
> from contributors frustrated with the lack of Project responsiveness and
> accountability. Stalled PRs have also been used in attempts to block
> features from being added to the next numbered release.

Interesting statement there. Particularly since you and I are
essentially the only reviewers.

Are you admitting reason for PR 358 not being approved yet?

I certainly have not done such underhanded politics. When I want to
block a feature for next release I state so clearly in the PR. Though
there is scant reason to block anything from merging to master when the
code is good - no numbered releases come from there.



> 
> While 100-day stalling is unheard of in other open source projects I
> know about, the problem with unresponsive reviewers is not unique to
> Squid. The best (applicable to the Squid Project) solution that I know
> of is a timeout:
> 
> If the reviewer remains unresponsive for N days, their negative review
> can be dismissed. The counting starts from a well-defined event S, and
> there are at least two reminder comments addressed at the reviewer (R1
> days after S and R2 days before the dismissal).
> 
> Do you think timeouts can solve the problem with stalled PRs if Project
> contributors do not attempt to game the system? Can you think of a
> better solution?
> 

I do not think this will solve stalled PRs.

It may lead to better communication when reviewers are forced to post
regularly about why no progress. But do we really prefer PRs littered
with a long history of that? or a clean history with the 'stalled' state
easily found?
 As the reviewer most likely to be hit by these notices, I much prefer
the PR to end with the thing needing action  than to have to read its
old history to figure out whether I can work on it immediately (see
above about the weekly scan through).
 We could use a tag set by the party suspecting a stall and unset by the
reviewer when they next pay attention to the PR.


Also, IMO it will just change to a different type of stall, add the risk
of Joe Blogs above, and revert Squid to the bygone situation where core
developers were regularly commiting code that broke trunk/master because
they personally thought it less broken than it was. We have this
negative as a veto to protect against those nasties - and for the most
part it is working.


I have outlined below what I think the real problem actually is.
Perhapse a re-focus on that can help come up with something that works
without the QA regressions.



> 
> Thank you,
> 
> Alex.
> P.S. A similar timeout approach can be applied to PRs stalled by
> authors, but those PRs represent a smaller problem and their incorrect
> handling results in little harm. We can consider that problem and argue
> about appropriate parameters for it later.



As I see the situation, the root cause is:

With multiple Factory personnel working paid (full time?) hours on Squid
code there are a lot of large and complicated Factory PRs to review and
only one non-Factory reviewer. When a non-Factory reviewer has an issue
with any one PR it is quickly responded to (usually not fixed,
apparently due to their commercial situation and sunken-costs?) and
bumped back into waiting-for-reviewer status.

Non-Factory patches come along relatively rarely and quickly handled by
the paid (almost-full time?) Factory reviewer with requests that require
a lot of author work.

So IMO these two reviewer vs author stalls are not distinct, but
directly linked to the originator of the PR.


[ In the spirit of being open, Alex is that Factory reviewer and I that
Non-Factory one. We both have quite a lot of overhead work like this
discussion to spread our Squid time over, so maybe hours:minutes instead
of days:hours depicted - I think the relative bias in time is the
important factor there rather than absolute units. ]


FYI: Github has some interesting stats in our records of PR approvals.
We essentially match up in number of PRs reviewed. Though there are some
bumps at the periods of the year my employment takes me away from Squid
where you process 1-2 more that month and those match up against the
times IIRC when the PR queue total grew and the mentioned stall PRs started.


Amos