IMHO, I don't think committers spend time watching new PRs coming up, but they more likely act when pinged. So, we may need some automation in case a contributor do not use github reviewed proposal. Auto reviewer assignment seem too much but modifying the PR template to add a sentence such as "please pickup a reviewer in the proposed list" could be enough. WDYT ?
and (1) A sentence in the PR template suggesting adding a reviewer. (easy) +100! Let's improve the message in the PR template. It costs nothing and can help a lot. I'd say it should be in bold letters as this is super important. Maybe this is also worth reconsidering if auto reviewer assignment (or at least some form of it) is a bad idea. We can assign committers (the most "hardcore" option, maybe too much) or ping them in emails/github comments if there's inactivity in pull requests (the soft one but requires a bot to be implemented). The way I see this is that such auto assigned reviewer could always say "I have lots on my plate" but suggest someone else to take care of the PR. This way the problem that nobody is mentioned by the PR author is completely gone. Other than that, such an approach feels efficient to me because it's more "in person" (similar to what Robert said). It's certainly disheartening as a reviewer to put time into reviewing a PR and then the author doesn't bother to even respond, or (as has happened to me) be told "hey, this wasn't ready for review yet." As for "this wasn't ready for review" - there are sometimes situations that require a PR to be opened before they are actually completed (especially when working with Jenkins jobs). Given that there might be misunderstandings authors of such commits should give a clear message saying "do not merge yet" or "not ready for review" in title or comments or even close such PR and reopen until the change is ready. It's all about giving a clear signal to others. Maybe we should mention it in guidelines/PR message too to avoid situations like this? Łukasz pon., 28 sty 2019 o 11:30 Robert Bradshaw <rober...@google.com> napisał(a): > On Mon, Jan 28, 2019 at 10:37 AM Etienne Chauchot <echauc...@apache.org> > wrote: > > > > Sure it's a pity than this PR got unnoticed and I think it is a > combination of factors (PR date around Christmas, the fact that the author > forgot - AFAIK - to ping a reviewer in either the PR or the ML). > > > > I agree with Rui's proposal to enhance visibility of the "how to get a > reviewed" process. > > > > IMHO, I don't think committers spend time watching new PRs coming up, > but they more likely act when pinged. So, we may need some automation in > case a contributor do not use github reviewed proposal. Auto reviewer > assignment seem too much but modifying the PR template to add a sentence > such as "please pickup a reviewer in the proposed list" could be enough. > > WDYT ? > > +1 > > I see two somewhat separable areas of improvement: > > (1) Getting a reviewer assigned to a PR, and > (2) Expectations of feedback/timeliness from a reviewer once it has > been assigned. > > The first is the motivation for this thread, but I think we're > suffering on the second as well. > > Given the reactions here, it sounds like most of us are just as > unhappy this happened as the author, and would be happy to pitch in > and improve things. > > I agree with Kenn that just adding to more the contributor guide > always help because a contributor guide with everything one might need > to know is the least likely to actually be read in its entirety. > Rather it's useful to provide such guidance at the point that it's > needed. Specifically, I would like to see > > (1) A sentence in the PR template suggesting adding a reviewer. (easy) > (2) An automated recommendation for suggesting good candidate > reviewers (if we deem Github's suggestions insufficient). (harder) > (3) A bot that follows up on PRs after 1 week(?) noting the lack of > action and suggesting (and, implicitly but importantly > permission/expectation) that the author bring the PR to the list. > (medium) > > We could also have automated emails like the Beam Dependency Check > Report, but automated emails are much easier to ignore than personal > ones. Having the author ping dev@ has the added advantage that it > gives the author something they can do to move the PR forward, and > provides a clear signal that this is a PR someone cares enough about > to prioritize it above others. (It's certainly disheartening as a > reviewer to put time into reviewing a PR and then the author doesn't > bother to even respond, or (as has happened to me) be told "hey, this > wasn't ready for review yet." On the other hand it's rewarding to help > someone, especially a first time contributor, to see their change > actually get in. Improving this ratio will I think both increase the > productivity of reviews and the motivation to do them.) > > > Also, I started to review the PR on Friday (thx Kenn for pinging me). > > > > Etienne > > > > Le vendredi 25 janvier 2019 à 10:21 -0800, Rui Wang a écrit : > > > > We have code contribution guidelines [1] and it says useful tips to make > PR reviewed and merged. But I guess it hides in Beam website so new > contributors are likely to ignore it. In order to make the guidance easy to > find and read for new contributors, we probably can > > > > a. Move number 5 item from [1] to a separate section and name it "Tips > to get your PR reviewed and merged" > > b. Put the link to the Github pull request template, so when a > contributor creates the first PR, the contributor could see the link (or > even paste text from contribution guide). It will be a good chance that new > contributors read what's in pull request template. > > > > > > -Rui > > > > [1] https://beam.apache.org/contribute/#make-your-change > > > > On Fri, Jan 25, 2019 at 9:24 AM Alexey Romanenko < > aromanenko....@gmail.com> wrote: > > > > For sure, it’s a pity that this PR has not been addressed for a long > time (I guess, we probably have other ones like this) but, as I can see > from this PR history, review has not been requested explicitly by author > (and this is one of the our recommendations for code contribution [1]). > > > > What are the options to improve this: > > > > 1) Make it more clearly for new contributors that they need to ask for a > review explicitly (with a help of recommendations that already provided in > top-right corner on PR page) > > 2) Create a bot (like “stale” bot that we have) to check for > non-addressed PRs that are more than, say, 7 days, and send notification to > dev@ (or dedicated, see n.3) mailing list if they are starving for review. > > 3) (Optionally) Create new mailing list called pr@ for new coming and > non-addressed PRs > > > > [1] https://beam.apache.org/contribute/#make-your-change > > > > > > On 25 Jan 2019, at 17:50, Ismaël Mejía <ieme...@gmail.com> wrote: > > > > The fact that this happened is a real pity. However it is clearly an > > exception and not the rule. Really few PRs have been long time without > > review. Can we somehow automatically send a notification if a PR has > > no assigned reviewers, or if it has not been reviewed after some time > > as Tim suggested? > > > > On Fri, Jan 25, 2019 at 9:43 AM Tim Robertson <timrobertson...@gmail.com> > wrote: > > > > > > Thanks Kenn > > > > I tend to think that timing is the main contributing factor as you note > on the Jira - it slipped down with no reminders / bumps sent on any > channels that I can see. > > > > Would something that alerts the dev@ list of PRs that have not received > any attention after N days be helpful perhaps? > > Even if that only prompts action by one of us to comment on the PR that > it's been acknowledged would likely be enough to engage the contributor - > they would hopefully then ping the individual if it then slips for a long > time. > > > > Next week will be my first I'll be able to work on Beam in 2019, but > I'll comment on that PR now too as it's missing tests. > > > > > > > > > > > > On Fri, Jan 25, 2019 at 7:27 AM Kenneth Knowles <k...@apache.org> wrote: > > > > > > The subject line is a quote from BEAM-6324* > > > > This makes me sad. I hope/expect it is a failure to route a pull request > to the right reviewer. I am less sad about the functionality than the > sentiment and how a contributor is being discouraged. > > > > Does anyone have ideas that could help? > > > > Kenn > > > > *https://issues.apache.org/jira/browse/BEAM-6324 > > > > >
