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 <fhue...@gmail.com> 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 <yazda...@gmail.com>: > >> 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 <t...@apache.org> 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 <yuzhih...@gmail.com> 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 <trohrm...@apache.org> >>>> 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 <ches...@apache.org> >>>>> 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 <fhue...@gmail.com> 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 <ches...@apache.org>: >>>>>>>> >>>>>>>> -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 <kisim...@163.com> >>>>> 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 >>>>>>>>>>> <pi...@data-artisans.com> 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 >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >> >>
