+1 for automatically closing. Currently, contribution guide mentions that pull requests without responses to actionable comments become stale (and closed) after two months but last I checked there were many pull requests that met this criteria but had not been closed after many months. So seems like humans are reluctant to act on the established policy :)
- Cham On Wed, May 23, 2018 at 11:25 AM Kenneth Knowles <[email protected]> wrote: > That makes sense, to just focus on Beam's decision. It seems the tool is > already built. I thought we just had to deploy it, but maybe not even that, > if we can just activate it: https://github.com/apps/stale > > Kenn > > On Wed, May 23, 2018 at 9:31 AM Ismaël Mejía <[email protected]> wrote: > >> Given that reaching consensus in both communities seems like a harder task >> than just deciding our policy. in the Beam side Why don't we just go ahead >> and vote around this + build the tool, and if the Flink guys are >> interested >> they can take it, no? >> >> in the future we can share that code. >> On Wed, May 16, 2018 at 3:31 PM Piotr Nowojski <[email protected]> >> wrote: >> >> > The question is what would such tool offer on top of over a Github’s >> view >> of PR sorted by “least recently updated”: >> >> >> >> https://github.com/apache/flink/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-asc >> <https://github.com/apache/flink/pulls?q=is:pr+is:open+sort:updated-asc> >> >> > ? Maybe it would be good enough to have a policy about waiting x >> months/weeks for a contributor to respond. If he doesn’t, we either take >> over PR we or close it. Having “clean” view of oldest PRs would be >> beneficial for reviewing PRs in ~FIFO order as part of community work. >> >> > Having >> >> > > On 16 May 2018, at 10:57, Fabian Hueske <[email protected]> wrote: >> > > >> > > Hi, >> > > >> > > I'm not objecting closing stale PRs. >> > > We have quite a few PRs with very little chance of being merged and I >> would >> > > certainly appreciate cleaning up those. >> > > However, I think we should not automate closing PRs for the reasons I >> gave >> > > before. >> > > >> > > A tool that reminds us of state PRs as proposed by Till seems like a >> good >> > > idea and might actually help to re-adjust priorities from time to >> time. >> > > >> > > @Yazdan: Right now there is no assignment happening. Committers decide >> > > which PRs they want to review and merge. >> > > >> > > Best, Fabian >> > > >> > > 2018-05-16 4:26 GMT+02:00 Yaz Sh <[email protected]>: >> > > >> > >> I have questions in this regard (you guys might have addressed it in >> this >> > >> email chain): >> > >> how PRs get assigned to a reviewer apart of a contributor tag >> someone? >> > >> what if PR never gets a reviewer attention and it became in-active >> due >> to >> > >> long review respond? should Bot assign a reviewer to a PR based on >> > >> reviewers interest (i.e., defined via tags) and notify he/she if PR >> is >> > >> waiting for review? >> > >> >> > >> >> > >> Cheers, >> > >> /Yazdan >> > >> >> > >> >> > >>> On May 15, 2018, at 12:06 PM, Thomas Weise <[email protected]> wrote: >> > >>> >> > >>> I like Till's proposal to notify the participants on the PR to PTAL. >> But >> > >> I >> > >>> would also suggest to auto-close when no action is taken, with a >> friendly >> > >>> reminder that PRs can be reopened anytime. >> > >>> >> > >>> The current situation with 350 open PRs may send a signal to >> contributors >> > >>> that it may actually be too much hassle to get a change committed in >> > >> Flink. >> > >>> Since the count keeps on rising, this is also not a past problem. >> Pruning >> > >>> inactive PRs may help, that will also give a more accurate picture >> of >> the >> > >>> lack of review bandwidth, if that is the root cause. >> > >>> >> > >>> Thanks, >> > >>> Thomas >> > >>> >> > >>> >> > >>> >> > >>> >> > >>> >> > >>> On Tue, May 15, 2018 at 8:24 AM, Ted Yu <[email protected]> >> wrote: >> > >>> >> > >>>> How does the bot decide whether the PR is waiting for reviews or is >> > >> being >> > >>>> abandoned by contributor ? >> > >>>> >> > >>>> How about letting the bot count the number of times contributor >> pings >> > >>>> committer(s) for review ? >> > >>>> When unanswered ping count crosses some threshold, say 3, the bot >> > >> publishes >> > >>>> the JIRA and PR somewhere. >> > >>>> >> > >>>> Cheers >> > >>>> >> > >>>> On Tue, May 15, 2018 at 8:19 AM, Till Rohrmann < >> [email protected]> >> > >>>> wrote: >> > >>>> >> > >>>>> I'm a bit torn here because I can see the pros and cons for both >> sides. >> > >>>>> >> > >>>>> Maybe a compromise could be to not have a closing but a monitoring >> bot >> > >>>>> which notifies us about inactive PRs. This could then trigger an >> > >>>>> investigation of the underlying problem and ultimately lead to a >> > >>>> conscious >> > >>>>> decision to close or keep the PR open. As such it is not strictly >> > >>>> necessary >> > >>>>> to have such a bot but it would at least remind us to make a >> decision >> > >>>> about >> > >>>>> older PRs with no activity. >> > >>>>> >> > >>>>> Cheers, >> > >>>>> Till >> > >>>>> >> > >>>>> On Tue, May 15, 2018 at 3:26 PM, Chesnay Schepler < >> [email protected]> >> > >>>>> wrote: >> > >>>>> >> > >>>>>> /So far I did it twice for older PRs. In both cases I didn’t get >> any >> > >>>>>> response and I even forgot in which PRs I had asked this >> question, >> so >> > >>>>> now I >> > >>>>>> can not even close them :S/ >> > >>>>>> >> > >>>>>> To be honest this sounds more like an issue with how your >> organize >> > >> your >> > >>>>>> work. No amount of closing PRs can fix that. >> > >>>>>> With GitBox we can assign reviewers to a PR, but I'm not sure >> whether >> > >>>> it >> > >>>>>> only allows committers to assign people. >> > >>>>>> Bookmarks or text files might help as well./ >> > >>>>>> / >> > >>>>>> >> > >>>>>> /Regarding only 30% blocked on contributor. I wonder what would >> be >> > >> this >> > >>>>>> number if we tried to ask in the rest of old PRs “Hey, are you >> still >> > >>>>>> interested in reviewing/merging this?”. If old PR is waiting >> for a >> > >>>>>> reviewer for X months, it doesn’t mean that’s not abandoned. Even >> if >> > >> it >> > >>>>>> doesn’t, reducing our overhead by those 30% of the PRs is >> something./ >> > >>>>>> >> > >>>>>> No doubt the number would be higher if we were to go back, but >> as i >> > >>>>>> explained earlier that is not a reason to close it. If a PR is >> > >>>> abandoned >> > >>>>>> because we messed up we should still /try /to get it in. >> > >>>>>> >> > >>>>>> /This is kind of whole point of what I was proposing. If the PR >> author >> > >>>> is >> > >>>>>> still there, and can respond/bump/interrupt the closing timeout, >> > >> that’s >> > >>>>>> great. Gives us even more sense of urgency to review it./ >> > >>>>>> >> > >>>>>> Unfortunately knowing that it is more urgent is irrelevant, as we >> > >>>>>> currently don't have the manpower to review them. Reviving them >> now >> > >>>> would >> > >>>>>> serve no purpose. The alternative is to wait until more people >> become >> > >>>>>> active reviewers. >> > >>>>>> >> > >>>>>> /As a last resort, closing PR after timeout is not the end of the >> > >>>> world. >> > >>>>>> It always can be reopened./ >> > >>>>>> >> > >>>>>> Let's be realistic here, it will not be reopened. >> > >>>>>> >> > >>>>>> >> > >>>>>> On 15.05.2018 14:21, Piotr Nowojski wrote: >> > >>>>>> >> > >>>>>>> I agree that we have other, even more important, problems with >> > >>>> reviewing >> > >>>>>>> PR and community, but that shouldn’t block us from trying to >> clean up >> > >>>>>>> things a little bit and minimise the effort needed for reviewing >> PRs. >> > >>>>> Now >> > >>>>>>> before reviewing/picking older PRs I had to ask this “Hey, are >> you >> > >>>> still >> > >>>>>>> interested in merging this?” manually and wait for the response. >> If >> > >> it >> > >>>>>>> doesn’t come, I have to remember to go back and close PR, which >> I >> of >> > >>>>> course >> > >>>>>>> forget to do. Bah, so far I did it twice for older PRs. In both >> cases >> > >>>> I >> > >>>>>>> didn’t get any response and I even forgot in which PRs I had >> asked >> > >>>> this >> > >>>>>>> question, so now I can not even close them :S Wasted effort and >> > >> wasted >> > >>>>> time >> > >>>>>>> on context switching for me and for everyone else that will have >> to >> > >>>>> scroll >> > >>>>>>> pass or look on those PR to check their status. >> > >>>>>>> >> > >>>>>>> Keep in mind that I am not proposing to close the PR >> automatically >> > >>>>>>> straight on after 3 months of inactivity. Only after asking a >> > >> question >> > >>>>>>> whether original contributor is still there and he is interested >> in >> > >>>> the >> > >>>>> PR >> > >>>>>>> to be reviewed. >> > >>>>>>> >> > >>>>>>> for Flink 1.5, I merged a contribution from PR #1990 after it >> was >> > >>>>>>>> requested a few times by users. >> > >>>>>>>> >> > >>>>>>> This is kind of whole point of what I was proposing. If the PR >> author >> > >>>> is >> > >>>>>>> still there, and can respond/bump/interrupt the closing timeout, >> > >>>> that’s >> > >>>>>>> great. Gives us even more sense of urgency to review it. On the >> other >> > >>>>> hand >> > >>>>>>> if there is no response (no interest from the author for >> whatever >> the >> > >>>>>>> reasons) and nobody so far has picked this PR to review/merge, >> what’s >> > >>>>> the >> > >>>>>>> point of keeping such PR open? As a last resort, closing PR >> after >> > >>>>> timeout >> > >>>>>>> is not the end of the world. It always can be reopened. >> > >>>>>>> >> > >>>>>>> Regarding only 30% blocked on contributor. I wonder what would >> be >> > >> this >> > >>>>>>> number if we tried to ask in the rest of old PRs “Hey, are you >> still >> > >>>>>>> interested in reviewing/merging this?”. If old PR is waiting >> for a >> > >>>>> reviewer >> > >>>>>>> for X months, it doesn’t mean that’s not abandoned. Even if it >> > >>>> doesn’t, >> > >>>>>>> reducing our overhead by those 30% of the PRs is something. >> > >>>>>>> >> > >>>>>>> Piotrek >> > >>>>>>> >> > >>>>>>> On 15 May 2018, at 10:10, Fabian Hueske <[email protected]> >> wrote: >> > >>>>>>>> >> > >>>>>>>> I'm with Chesnay on this issue. >> > >>>>>>>> Stale PRs, i.e., a PR where a contributor becomes inactive, are >> one >> > >>>> of >> > >>>>>>>> our >> > >>>>>>>> smallest issues, IMO. >> > >>>>>>>> >> > >>>>>>>> There are more reasons for the high number of PRs. >> > >>>>>>>> * Lack of timely reviews. >> > >>>>>>>> * Not eagerly closing PRs that have no or very little chance of >> > >> being >> > >>>>>>>> merged. Common reasons are >> > >>>>>>>> 1) lack of interest in the feature by committers, >> > >>>>>>>> 2) too extensive changes and hence time consuming reviews, or >> > >>>>>>>> 3) bad quality. >> > >>>>>>>> >> > >>>>>>>> For 1), there are lots of older JIRA issues, that have low >> priority >> > >>>> but >> > >>>>>>>> which are picked up by contributors. In the contribution >> guidelines >> > >>>> we >> > >>>>>>>> ask >> > >>>>>>>> contributors to let us know when they want to work on an issue. >> So >> > >>>> far >> > >>>>>>>> our >> > >>>>>>>> attitude has been, yes sure go ahead. I've seen very little >> attempts >> > >>>> of >> > >>>>>>>> warning somebody to work on issues that won't be easily merged. >> > >>>>>>>> Once a PR has been opened, we should also be honest and let >> > >>>>> contributors >> > >>>>>>>> know that it has no chance or might take a while to get >> reviewed. >> > >>>>>>>> For 2) this is typically not so much of an issue if the feature >> is >> > >>>>>>>> interesting. However, if 1) and 2) meet, chances to get a >> change >> in >> > >>>>> drop >> > >>>>>>>> even more. >> > >>>>>>>> >> > >>>>>>>> A common "strategy" to deal with PRs that fall into 1), 2), or >> 3) is >> > >>>> to >> > >>>>>>>> not >> > >>>>>>>> look at them or giving a shallow review. >> > >>>>>>>> Of course, contributors become unresponsive if we don't look at >> > >> their >> > >>>>> PRs >> > >>>>>>>> for weeks or months. But that's not their fault. >> > >>>>>>>> Instead, I think we should be honest and communicate the >> chances >> of >> > >> a >> > >>>>> PR >> > >>>>>>>> more clearly. >> > >>>>>>>> >> > >>>>>>>> Browsing over the list of open PRs, I feel that most older PRs >> fall >> > >>>>> into >> > >>>>>>>> the category of low-priority (or even unwanted) features. >> > >>>>>>>> Bug fixes or features that the committers care about usually >> make it >> > >>>>> into >> > >>>>>>>> the code base. >> > >>>>>>>> In case a contributor becomes inactive, committers often take >> over >> > >> an >> > >>>>>>>> push >> > >>>>>>>> a contribution over the line. >> > >>>>>>>> >> > >>>>>>>> So, I do not support an auto-close mechanism. >> > >>>>>>>> I think a better way to address the issue is better >> communication, >> > >>>> more >> > >>>>>>>> eagerly closing PRs with no chance, and putting more reviewing >> > >>>> effort. >> > >>>>>>>> IMO, we should only eagerly close PRs that have no chance of >> being >> > >>>>>>>> merged. >> > >>>>>>>> PRs with low-prio features might be picked up later (for Flink >> 1.5, >> > >> I >> > >>>>>>>> merged a contribution from PR #1990 after it was requested a >> few >> > >>>> times >> > >>>>> by >> > >>>>>>>> users). >> > >>>>>>>> >> > >>>>>>>> However, I think we could do a pass over the oldest PRs and >> check if >> > >>>> we >> > >>>>>>>> can >> > >>>>>>>> close a bunch. >> > >>>>>>>> There are quite a few contributions (many for flink-ml) that I >> don't >> > >>>>> see >> > >>>>>>>> a >> > >>>>>>>> chance for getting merged. >> > >>>>>>>> >> > >>>>>>>> Best, Fabian >> > >>>>>>>> >> > >>>>>>>> >> > >>>>>>>> - >> > >>>>>>>> >> > >>>>>>>> 2018-05-15 9:13 GMT+02:00 Chesnay Schepler <[email protected] >> >: >> > >>>>>>>> >> > >>>>>>>> -1 >> > >>>>>>>>> >> > >>>>>>>>> For clarification (since the original mail indicates >> otherwise), >> > >> the >> > >>>>>>>>> number of pull requests that this would affect is fairly >> small. >> > >>>>>>>>> Only about 25-30% of all open PRs are blocked on the >> contributor, >> > >>>> the >> > >>>>>>>>> remaining ones are actually blocked on the review. >> > >>>>>>>>> Thus is reject the premise that one has to search through that >> many >> > >>>>> PRs >> > >>>>>>>>> to >> > >>>>>>>>> find something to review, there is plenty. >> > >>>>>>>>> >> > >>>>>>>>> I believe it to be highly unfair for us to close PRs due to >> > >>>>> inactivity, >> > >>>>>>>>> when the primary cause has been /our /own inactivity. >> > >>>>>>>>> If a PR is opened and the first comment comes in 3 months >> late, >> > >>>> then I >> > >>>>>>>>> don't blame the contributor for not responding. >> > >>>>>>>>> IMO we owe it to the contributor to evaluate their PR, and if >> > >>>>> necessary >> > >>>>>>>>> bring it to a merge-able state (to a certain extend). >> > >>>>>>>>> >> > >>>>>>>>> There's also the fact that closing these PRs outright would >> waste a >> > >>>>> lot >> > >>>>>>>>> of >> > >>>>>>>>> good contributions. >> > >>>>>>>>> >> > >>>>>>>>> Finally, this solution is overkill for what we want to >> achieve. >> If >> > >>>> we >> > >>>>>>>>> want >> > >>>>>>>>> to make it easier to find PRs to review all we need is >> > >>>>>>>>> GitBox integration and tagging or PRs. That's it. We could >> have >> a >> > >>>>> /fully >> > >>>>>>>>> /tagged PR list /tomorrow/, if we really wanted to. >> > >>>>>>>>> >> > >>>>>>>>> >> > >>>>>>>>> On 15.05.2018 05:10, Ted Yu wrote: >> > >>>>>>>>> >> > >>>>>>>>> bq. this pull request requires a review, please simply write >> any >> > >>>>>>>>>> comment. >> > >>>>>>>>>> >> > >>>>>>>>>> Shouldn't the wording of such comment be known before hand ? >> > >>>>>>>>>> >> > >>>>>>>>>> Otherwise pull request waiting for committers' review may be >> > >>>>>>>>>> mis-classified. >> > >>>>>>>>>> >> > >>>>>>>>>> Cheers >> > >>>>>>>>>> >> > >>>>>>>>>> On Mon, May 14, 2018 at 7:59 PM, blues zheng < >> [email protected]> >> > >>>>> wrote: >> > >>>>>>>>>> >> > >>>>>>>>>> +1 for the proposal. >> > >>>>>>>>>> >> > >>>>>>>>>>> >> > >>>>>>>>>>> Best, >> > >>>>>>>>>>> blues >> > >>>>>>>>>>> On 05/14/2018 20:58, Ufuk Celebi wrote: >> > >>>>>>>>>>> Hey Piotr, >> > >>>>>>>>>>> >> > >>>>>>>>>>> thanks for bringing this up. I really like this proposal and >> also >> > >>>>> saw >> > >>>>>>>>>>> it work successfully at other projects. So +1 from my side. >> > >>>>>>>>>>> >> > >>>>>>>>>>> - I like the approach with a notification one week before >> > >>>>>>>>>>> automatically closing the PR >> > >>>>>>>>>>> - I think a bot will the best option as these kinds of >> things >> are >> > >>>>>>>>>>> usually followed enthusiastically in the beginning but >> eventually >> > >>>>>>>>>>> loose traction >> > >>>>>>>>>>> >> > >>>>>>>>>>> We can enable better integration with GitHub by using ASF >> GitBox >> > >>>>>>>>>>> (https://gitbox.apache.org/setup/) but we should discuss >> that >> in >> > >>>> a >> > >>>>>>>>>>> separate thread. >> > >>>>>>>>>>> >> > >>>>>>>>>>> – Ufuk >> > >>>>>>>>>>> >> > >>>>>>>>>>> On Mon, May 14, 2018 at 12:04 PM, Piotr Nowojski >> > >>>>>>>>>>> <[email protected]> wrote: >> > >>>>>>>>>>> >> > >>>>>>>>>>> Hey, >> > >>>>>>>>>>>> >> > >>>>>>>>>>>> We have lots of open pull requests and quite some of them >> are >> > >>>>>>>>>>>> >> > >>>>>>>>>>>> stale/abandoned/inactive. Often such old PRs are impossible >> to >> > >>>>> merge >> > >>>>>>>>>>> due >> > >>>>>>>>>>> to >> > >>>>>>>>>>> conflicts and it’s easier to just abandon and rewrite them. >> > >>>>> Especially >> > >>>>>>>>>>> there are some PRs which original contributor created long >> time >> > >>>> ago, >> > >>>>>>>>>>> someone else wrote some comments/review and… that’s about >> it. >> > >>>>> Original >> > >>>>>>>>>>> contributor never shown up again to respond to the comments. >> > >>>>>>>>>>> Regardless >> > >>>>>>>>>>> of >> > >>>>>>>>>>> the reason such PRs are clogging the GitHub, making it >> difficult >> > >>>> to >> > >>>>>>>>>>> keep >> > >>>>>>>>>>> track of things and making it almost impossible to find a >> little >> > >>>> bit >> > >>>>>>>>>>> old >> > >>>>>>>>>>> (for example 3+ months) PRs that are still valid and waiting >> for >> > >>>>>>>>>>> reviews. >> > >>>>>>>>>>> To do something like that, one would have to dig through >> tens >> or >> > >>>>>>>>>>> hundreds >> > >>>>>>>>>>> of abandoned PRs. >> > >>>>>>>>>>> >> > >>>>>>>>>>> What I would like to propose is to agree on some inactivity >> dead >> > >>>>> line, >> > >>>>>>>>>>>> >> > >>>>>>>>>>>> lets say 3 months. After crossing such deadline, PRs should >> be >> > >>>>>>>>>>> marked/commented as “stale”, with information like: >> > >>>>>>>>>>> >> > >>>>>>>>>>> “This pull request has been marked as stale due to 3 months >> of >> > >>>>>>>>>>>> >> > >>>>>>>>>>>> inactivity. It will be closed in 1 week if no further >> activity >> > >>>>>>>>>>> occurs. If >> > >>>>>>>>>>> you think that’s incorrect or this pull request requires a >> > >> review, >> > >>>>>>>>>>> please >> > >>>>>>>>>>> simply write any comment.” >> > >>>>>>>>>>> >> > >>>>>>>>>>> Either we could just agree on such policy and enforce it >> manually >> > >>>>>>>>>>>> (maybe >> > >>>>>>>>>>>> >> > >>>>>>>>>>>> with some simple tooling, like a simple script to list >> inactive >> > >>>>> PRs - >> > >>>>>>>>>>> seems >> > >>>>>>>>>>> like couple of lines in python by using PyGithub) or we >> could >> > >>>> think >> > >>>>>>>>>>> about >> > >>>>>>>>>>> automating this action. There are some bots that do exactly >> this >> > >>>>> (like >> > >>>>>>>>>>> this >> > >>>>>>>>>>> one: https://github.com/probot/stale < >> https://github.com/probot/ >> > >>>>> stale >> > >>>>>>>>>>>> >> > >>>>>>>>>>> ), >> > >>>>>>>>>>> but probably they would need to be adopted to limitations of >> our >> > >>>>>>>>>>> Apache >> > >>>>>>>>>>> repository (we can not add labels and we can not close the >> PRs >> > >> via >> > >>>>>>>>>>> GitHub). >> > >>>>>>>>>>> >> > >>>>>>>>>>> What do you think about it? >> > >>>>>>>>>>>> >> > >>>>>>>>>>>> Piotrek >> > >>>>>>>>>>>> >> > >>>>>>>>>>>> >> > >>>>>>>>> >> > >>>>>>> >> > >>>>>> >> > >>>>> >> > >>>> >> > >> >> > >> >> >
