Wow. I missed the sentence. Judging from the fact that others also proposed adding it, I think it might need some care. I proposed a PR here: https://github.com/apache/beam/pull/7670
Łukasz śr., 30 sty 2019 o 00:39 Kenneth Knowles <k...@google.com> napisał(a): > > > 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 >>> > >>> > >>> >>
