Hi David, I checked the PR, looks good! And I think the times set are adequate.
Best, ------------------ Josep Prat Open Source Engineering Director, Aiven josep.p...@aiven.io | +491715557497 | aiven.io Aiven Deutschland GmbH Alexanderufer 3-7, 10117 Berlin Geschäftsführer: Oskari Saarenmaa, Hannu Valtonen, Anna Richardson, Kenneth Chen Amtsgericht Charlottenburg, HRB 209739 B On Wed, Sep 11, 2024, 19:51 David Arthur <mum...@gmail.com> wrote: > 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 > <https://www.google.com/maps/search/Josep+Prat+?entry=gmail&source=g> > <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 somet > <https://www.google.com/maps/search/der+if+we+also+need+to+do+somet?entry=gmail&source=g>hing > 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 > <https://www.google.com/maps/search/become+stale,+and+I+guess+are+?entry=gmail&source=g>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 >
