Hey all, I just wanted to bump this one more time before I merge this PR (thanks for the review, Josep!). I'll merge it at the end of the day today unless anyone has more feedback.
Thanks! David On Wed, Jun 7, 2023 at 8:50 PM David Arthur <mum...@gmail.com> wrote: > I filed KAFKA-15073 for this. Here is a patch > https://github.com/apache/kafka/pull/13827. This simply adds a "stale" > label to PRs with no activity in the last 90 days. I figure that's a good > starting point. > > As for developer workflow, the "stale" action is quite flexible in how it > finds candidate PRs to mark as stale. For example, we can exclude PRs that > have an Assignee, or a particular set of labels. Docs are here > https://github.com/actions/stale > > -David > > > On Wed, Jun 7, 2023 at 2:36 PM Josep Prat <josep.p...@aiven.io.invalid> > wrote: > > > Thanks David! > > > > ——— > > 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 Wed, Jun 7, 2023, 20:28 David Arthur <david.art...@confluent.io > > .invalid> > > wrote: > > > > > Hey all, I started poking around at Github actions on my fork. > > > > > > https://github.com/mumrah/kafka/actions > > > > > > I'll post a PR if I get it working and we can discuss what kind of > > settings > > > we want (or if we want it all) > > > > > > -David > > > > > > On Tue, Jun 6, 2023 at 1:18 PM Chris Egerton <chr...@aiven.io.invalid> > > > wrote: > > > > > > > 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 > > > > > > > > > > > > > > > > > > -- > > > -David > > > > > > > > -- > David Arthur > -- -David
