flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Haohui Mai <ricet...@gmail.com>
Subject Re: [DISCUSS] (Not) tagging reviewers
Date Thu, 26 Jan 2017 05:58:33 GMT
+1. Nice to be pinged if required, but relying on specific people to review
simply does not scale.

Popping up one level, occasionally the fact that people tag other people is
because the PRs are left unreviewed for a while (or they believe the PRs
will not be reviewed unless they explicitly ping other folks). Having more
people (not necessarily committers) to participate the review process will
go a long way.

I took a quick skim on the PRs and I noticed that only a few of them are
actually in mergeable shapes (i.e., properly rebased and passing CI).

Maybe it is a good idea to proactively clean up the open pull requests to
make it easier for people to check them out? That way there might be more
help for reviews from the community side.

Regards,
Haohui

On Tue, Jan 24, 2017 at 5:07 AM Till Rohrmann <trohrmann@apache.org> wrote:

> I agree with Aljoscha that tagging the PRs helps me to get notified about
> PRs where I could be of help. But I also think that it should not be the
> ultimate responsibility of the tagged person(s) to review the code.
> Everyone should feel empowered to do so. And also the tagging does not free
> oneself from browsing the PR list to check out the open PRs.
>
> @Theo, usually this notification should already happen via the JIRA
> integration given that the person who created the JIRA issue also watches
> it.
>
> Cheers,
> Till
>
> On Tue, Jan 24, 2017 at 1:29 PM, Theodore Vasiloudis <
> theodoros.vasiloudis@gmail.com> wrote:
>
> > I was wondering how this relates to the shepherding of PRs we have
> > discussed in the past. If I make a PR for an issue reported from a
> specific
> > committer, doesn't tagging them make sense?
> >
> > Has the shepherding of PRs been tried out?
> >
> > On Tue, Jan 24, 2017 at 12:17 PM, Aljoscha Krettek <aljoscha@apache.org>
> > wrote:
> >
> > > It seems I'm in a bit of a minority here but I like the @R tags. There
> > are
> > > simply to many pull request for someone to keep track of all of them
> and
> > if
> > > someone things that a certain person would be good for reviewing a
> change
> > > then tagging them helps them notice the PR.
> > >
> > > I think the tag should not mean that only that person can/should review
> > the
> > > PR, it should serve as a proposal.
> > >
> > > I'm happy to not use it anymore if everyone else doesn't like them.
> > >
> > > On Sat, 21 Jan 2017 at 00:53 Fabian Hueske <fhueske@gmail.com> wrote:
> > >
> > > > Hi Haohui,
> > > >
> > > > reviewing pull requests is a great way of contributing to the
> > community!
> > > >
> > > > I am not aware of specific instructions for the review process. The
> are
> > > > some dos and don'ts on our "contribute code" page [1] that should be
> > > > considered. Apart from that, I think the best way to start is to
> become
> > > > familiar with a certain part of the code base (reading code,
> > > contributing)
> > > > and then to look out for pull requests that address the part you are
> > > > familiar with.
> > > >
> > > > The review does not have to cover all aspects of a PR (a committer
> will
> > > > have a look as well), but from my personal experience the effort to
> > > review
> > > > a PR is often much lower if some other person has had a look at it
> > > already
> > > > and gave feedback.
> > > > I think this can help a lot to reduce the review "load" on the
> > > committers.
> > > > Maybe you find some contributors who are interested in the same
> > > components
> > > > as you and you can start reviewing each others code.
> > > >
> > > > Thanks,
> > > > Fabian
> > > >
> > > > [1] http://flink.apache.org/contribute-code.html#coding-guidelines
> > > >
> > > >
> > > > 2017-01-20 23:02 GMT+01:00 jincheng sun <sunjincheng121@gmail.com>:
> > > >
> > > > > I totally agree with all of your ideas.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Best wishes,
> > > > >
> > > > >
> > > > >
> > > > > SunJincheng.
> > > > >
> > > > > Stephan Ewen <sewen@apache.org>于2017年1月16日 周一19:42写道:
> > > > >
> > > > > > Hi!
> > > > > >
> > > > > >
> > > > > >
> > > > > > I have seen that recently many pull requests designate reviews
by
> > > > writing
> > > > > >
> > > > > > "@personA review please" or so.
> > > > > >
> > > > > >
> > > > > >
> > > > > > I am personally quite strongly against that, I think it hurts
the
> > > > > community
> > > > > >
> > > > > > work:
> > > > > >
> > > > > >
> > > > > >
> > > > > >   - The same few people get usually "designated" and will
> typically
> > > get
> > > > > >
> > > > > > overloaded and often not do the review.
> > > > > >
> > > > > >
> > > > > >
> > > > > >   - At the same time, this discourages other community members
> from
> > > > > looking
> > > > > >
> > > > > > at the pull request, which is totally undesirable.
> > > > > >
> > > > > >
> > > > > >
> > > > > >   - In general, review participation should be "pull based"
> (person
> > > > > decides
> > > > > >
> > > > > > what they want to work on) not "push based" (random person pushes
> > > work
> > > > to
> > > > > >
> > > > > > another person). Push-based just creates the wrong feeling in
a
> > > > community
> > > > > >
> > > > > > of volunteers.
> > > > > >
> > > > > >
> > > > > >
> > > > > >   - In many cases the designated reviews are not the ones most
> > > > > >
> > > > > > knowledgeable in the code, which is understandable, because
how
> > > should
> > > > > >
> > > > > > contributors know whom to tag?
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Long story short, why don't we just drop that habit?
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > Greetings,
> > > > > >
> > > > > > Stephan
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message