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 >
