Hey folks, I wanted to revive this old thread. I'd like to do the following:
* Change our stale workflow to start with the oldest PRs and move forward * Enable closing of stale PRs (over 120 days) Here's a patch with these changes: https://github.com/apache/kafka/pull/17166 Docs for actions/stale: https://github.com/actions/stale Cheers, David A On Sat, Jun 10, 2023 at 2:53 AM David Jacot <david.ja...@gmail.com> wrote: > Thanks, David. I left a few comments in the PR. > > -David > > Le ven. 9 juin 2023 à 15:31, David Arthur <david.art...@confluent.io > .invalid> > a écrit : > > > 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 > > < > https://www.google.com/maps/search/%3E+%3E%3E+%3E%3E%3E%3E+when+a+person+told+me?entry=gmail&source=g > > > > 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 > > > -- David Arthur
