Hi Josep, Thanks for bringing this up! Will try to keep things brief.
I'm generally in favor of this initiative. A couple ideas that I really liked: requiring a component label (producer, consumer, connect, streams, etc.) before closing, and disabling auto-close (i.e., automatically tagging PRs as stale, but leaving it to a human being to actually close them). We might replace the "stale" label with a "close-by-<date>" label so that it becomes even easier for us to find the PRs that are ready to be closed (as opposed to the ones that have just been labeled as stale without giving the contributor enough time to respond). I've also gone ahead and closed some of my stale PRs. Others I've downgraded to draft to signal that I'd like to continue to pursue them, but have to iron out merge conflicts first. For the last ones, I'll ping for review. One question that came to mind--do we want to distinguish between regular and draft PRs? I'm guessing not, since they still add up to the total PR count against the project, but since they do also implicitly signal that they're not intended for review (yet) it may be friendlier to leave them up. Cheers, Chris On Tue, Jun 6, 2023 at 10:18 AM Mickael Maison <mickael.mai...@gmail.com> wrote: > Hi Josep, > > Thanks for looking into this. This is clearly one aspect where we need > to improve. > > We had a similar initiative last year > (https://lists.apache.org/thread/66yj9m6tcyz8zqb3lqlbnr386bqwsopt) and > we closed many PRs. Unfortunately we did not follow up with a process > or automation and we are back to the same situation. > > Manually reviewing all these PRs is a huge task, so I think we should > at least partially automate it. I'm not sure if we should manually > review the oldest PRs (pre 2020). There's surely many interesting > things but I wonder if we should instead focus on the more recent ones > as they have a higher chance of 1) still making sense, 2) getting > updates from their authors, 3) needing less rebasing. If something has > been broken since 2016 but we never bothered to fix the PR it means it > can't be anything critical! > > Finally as Colin mentioned, it looks like a non negligible chunk of > stale PRs comes from committers and regular contributors. So I'd > suggest we each try to clean our own backlog too. > > I wonder if we also need to do something in JIRA. Querying for > unresolved tickets returns over 4000 items. Considering we're not > quite at KAFKA-15000 yet, that's a lot. > > Thanks, > Mickael > > > On Tue, Jun 6, 2023 at 11:35 AM Josep Prat <josep.p...@aiven.io.invalid> > wrote: > > > > Hi Devs, > > I would say we can split the problem in 2. > > > > Waiting for Author feedback: > > We could have a bot that would ping authors for the cases where we have > PRs > > that are stalled and have either: > > - Merge conflict > > - Unaddressed reviews > > > > Waiting for reviewers: > > For the PRs where we have no reviewers and there are no conflicts, I > think > > we would need some human interaction to determine modules (maybe this can > > be automated) and ping people who could review. > > > > What do you think? > > > > Best, > > > > On Tue, Jun 6, 2023 at 11:30 AM Josep Prat <josep.p...@aiven.io> wrote: > > > > > Hi Nikolay, > > > > > > With a bot it will be complicated to determine what to do when the PR > > > author is waiting for a reviewer. If a person goes over them, can > check if > > > they are waiting for reviews and tag the PR accordingly and maybe ping > a > > > maintainer. > > > If you look at my last email I described a flow (but AFAIU it will work > > > only if a human executes it) where the situation you point out would be > > > covered. > > > > > > ——— > > > Josep Prat > > > > > > Aiven Deutschland GmbH > > > > > > Alexanderufer 3-7, 10117 Berlin > > > > > > Amtsgericht Charlottenburg, HRB 209739 B > > > > > > Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > > > > > > m: +491715557497 > > > > > > w: aiven.io > > > > > > e: josep.p...@aiven.io > > > > > > On Tue, Jun 6, 2023, 11:20 Николай Ижиков <nizhi...@apache.org> wrote: > > > > > >> Hello. > > >> > > >> What is actions of contributor if no feedback given? [1], [2] > > >> > > >> [1] https://github.com/apache/kafka/pull/13278 > > >> [2] https://github.com/apache/kafka/pull/13247 > > >> > > >> > 2 июня 2023 г., в 23:38, David Arthur <mum...@gmail.com> > написал(а): > > >> > > > >> > I think this is a great idea. If we don’t want the auto-close > > >> > functionality, we can set it to -1 > > >> > > > >> > I realize this isn’t a vote, but I’m +1 on this > > >> > > > >> > -David > > >> > > > >> > On Fri, Jun 2, 2023 at 15:34 Colin McCabe <cmcc...@apache.org> > wrote: > > >> > > > >> >> That should read "30 days without activity" > > >> >> > > >> >> (I am assuming we have the ability to determine when a PR was last > > >> updated > > >> >> on GH) > > >> >> > > >> >> best, > > >> >> Colin > > >> >> > > >> >> On Fri, Jun 2, 2023, at 12:32, Colin McCabe wrote: > > >> >>> Hi all, > > >> >>> > > >> >>> Looking at GitHub, I have a bunch of Kafka PRs of my own that I've > > >> >>> allowed to become stale, and I guess are pushing up these numbers! > > >> >>> Overall I support the goal of putting a time limit on PRs, just so > > >> that > > >> >>> we can focus our review bandwidth. > > >> >>> > > >> >>> It may be best to start with a simple approach where we mark PRs > as > > >> >>> stale after 30 days and email the submitter at that time. And then > > >> >>> delete after 60 days. (Of course the exact time periods might be > > >> >>> something gother than 30/60 but this is just an initial > suggestion) > > >> >>> > > >> >>> best, > > >> >>> Colin > > >> >>> > > >> >>> > > >> >>> On Fri, Jun 2, 2023, at 00:37, Josep Prat wrote: > > >> >>>> Hi all, > > >> >>>> > > >> >>>> I want to say that in my experience, I always felt better as a > > >> >> contributor > > >> >>>> when a person told me something than when a bot did. That being > said, > > >> >> I'm > > >> >>>> not against bots, and I think they might be a great solution > once we > > >> >> have a > > >> >>>> manageable number of open PRs. > > >> >>>> > > >> >>>> Another great question that adding a bot poses is types of > staleness > > >> >>>> detection. How do we distinguish between staleness from the > author's > > >> >> side > > >> >>>> or from the lack of reviewers/maintainers' side? That's why I > started > > >> >> with > > >> >>>> a human approach to be able to distinguish between these 2 cases. > > >> Both > > >> >>>> situations should have different messages and actually different > > >> >> intended > > >> >>>> receivers. In case of staleness because of author inactivity, the > > >> >> message > > >> >>>> should encourage the author to update the PR with the requested > > >> changes > > >> >> or > > >> >>>> resolve the conflicts. But In many cases, PRs are stale because > of > > >> lack > > >> >> of > > >> >>>> reviewers. This would need a different message, targeting > > >> maintainers. > > >> >>>> > > >> >>>> Ideally (with bot or not) I believe the process should be as > follows: > > >> >>>> - Check PRs that are stale. > > >> >>>> - See if they have labels, if not add proper labels (connect, > core, > > >> >>>> clients...) > > >> >>>> - PR has merge conflicts > > >> >>>> -- Merge conflicts exist and target files that still exist, ping > the > > >> >> author > > >> >>>> asking for conflict resolution and add some additional label like > > >> >> `stale`. > > >> >>>> -- Merge conflicts exist and target files that do not exist > anymore, > > >> let > > >> >>>> the author know that this PR is obsolete, label the PR as > 'obsolete' > > >> and > > >> >>>> close the PR. > > >> >>>> - PR is mergeable, check whose action is needed (author or > reviewers) > > >> >>>> -- Author: let the author know that there are pending comments to > > >> >> address. > > >> >>>> Add some additional label, maybe `stale` again > > >> >>>> -- Reviewer: ping some reviewers that have experience or lately > > >> touched > > >> >>>> this piece of the codebase, add a label `reviewer needed` or > > >> something > > >> >>>> similar > > >> >>>> - PRs that have `stale` label after X days, will be closed. > > >> >>>> > > >> >>>> Regarding the comments about only committers and collaborators > being > > >> >> able > > >> >>>> to label PRs, this is true, not everyone can do this. However, > this > > >> >> could > > >> >>>> be a great opportunity for the newly appointed contributors to > > >> exercise > > >> >>>> their new permissions :) > > >> >>>> > > >> >>>> Let me know if it makes sense to you all. > > >> >>>> > > >> >>>> Best, > > >> >> > > >> > -- > > >> > David Arthur > > >> > > >> > > > > -- > > [image: Aiven] <https://www.aiven.io> > > > > *Josep Prat* > > Open Source Engineering Director, *Aiven* > > josep.p...@aiven.io | +491715557497 > > aiven.io <https://www.aiven.io> | < > https://www.facebook.com/aivencloud> > > <https://www.linkedin.com/company/aiven/> < > https://twitter.com/aiven_io> > > *Aiven Deutschland GmbH* > > Alexanderufer 3-7, 10117 Berlin > > Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > > Amtsgericht Charlottenburg, HRB 209739 B >
