On Mon, Jan 28, 2019 at 10:37 AM Etienne Chauchot <[email protected]> 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 <[email protected]> > 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 <[email protected]> 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 <[email protected]> > 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 <[email protected]> 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 > >
