On Mon, Jan 28, 2019 at 5:25 AM Łukasz Gajowy <lgaj...@apache.org> wrote:
> 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. > There is already a message. Is it unclear? Can you rephrase it to something better? Kenn > 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 >> > >> > >> >
