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 _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev