I also missed the sentence Kenn mentioned. I think it is worth enlightening it.
Thx for your PR around that Lukasz !
Etienne
Le mercredi 30 janvier 2019 à 11:03 +0100, Łukasz Gajowy a écrit :
> 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
> > > > >
> > > > >
> > > >
