flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Fabian Hueske <fhue...@gmail.com>
Subject Re: [DISCUSS] Introducing a review process for pull requests
Date Mon, 19 Oct 2015 15:53:05 GMT
Thanks for your feedback, Alex.

Chesnay is right, we cannot modify the GH assignee field at the moment. If
this changes at some point, I would support your proposal.

Regarding the PR - JIRA rule, this has been discussed as part of the new
contribution guidelines discussion [1].
I agree, it is not always easy to draw a line here. However, if in doubt I
would go for the JIRA issue because it allows everybody to track the issue
later, esp. if it solves a bug that other people might run into as well.
In your concrete example, you could have referenced the original JIRA issue
to remove Spargel from your PR.

I also agree that the shepherd should not be the author of the PR. However,
every committer can always commit to the master branch (unless somebody
raised concerns of course). So committers should be free to commit their
own PRs, IMO.

Cheers, Fabian

[1]
http://mail-archives.apache.org/mod_mbox/flink-dev/201509.mbox/%3CCAAdrtT0RLgqJgJVrK1zH_e_7VW1k8VTv4ChnumTRh7fLGA_zmw%40mail.gmail.com%3E


2015-10-17 13:00 GMT+02:00 Alexander Alexandrov <
alexander.s.alexandrov@gmail.com>:

> Also, regarding the "Each PR should be backed by a JIRA" rule - please
> don't make this strict and consider the relative effort to opening a JIRA
> as opposed to just opening a PR for small PRs that fix something obvious.
>
> For example, two days ago I opened two PRs while investigating a reported
> JIRA bug (FLINK-2858 <https://issues.apache.org/jira/browse/FLINK-2858>):
>
> https://github.com/apache/flink/pull/1259
> https://github.com/apache/flink/pull/1260
>
> 1259 removes obsolete references to the (now removed 'flink-spargel' code
> from the POM), while 1260 updates a paragraph of the "How to Build"
> documentation and fixes some broken href anchors.
>
> Just my two cents here, but fixes (not only hotfixes) that
>
> * resolve minor and obvious issues with the existing code or the
> documentation,
> * can be followed by everybody just by looking at the diff
>
> should be just cherry-picked (and if needed amended) by a committer without
> too much unnecessary discussion and excluded from the "shepherding
> process".
>
>
> 2015-10-17 12:32 GMT+02:00 Alexander Alexandrov <
> alexander.s.alexandrov@gmail.com>:
>
> > One suggestion from me: in GitHub you can make clear who the current
> > sheppard is through the "Assignee" field in the PR (which can and IMHO
> > should be different from the user who actually opened the request).
> >
> > Regards,
> > A.
> >
> > 2015-10-16 15:58 GMT+02:00 Fabian Hueske <fhueske@gmail.com>:
> >
> >> Hi folks,
> >>
> >> I think we can split of the discussion of a PR meeting.
> >>
> >> Are there any more comments on the proposal itself?
> >> Otherwise, I would go ahead and put the described process (modulo the
> >> comments) into a document for our website.
> >>
> >> Cheers, Fabian
> >>
> >> 2015-10-07 16:57 GMT+02:00 Fabian Hueske <fhueske@gmail.com>:
> >>
> >> > I like the idea of meeting once a week to discuss about PRs as well.
> >> >
> >> > Regarding lingering PRs, you can simply sort the Flink PRs in Github
> by
> >> > "least recently updated"
> >> >
> >> > Cheers,
> >> > Fabian
> >> >
> >> > 2015-10-07 16:48 GMT+02:00 Theodore Vasiloudis <
> >> > theodoros.vasiloudis@gmail.com>:
> >> >
> >> >> >
> >> >> > Could we maybe do a "PR overall status assessment" once per week
or
> >> so,
> >> >> > where we find those problematic PRs and try to assign them / close
> >> them?
> >> >>
> >> >>
> >> >> I like this idea, as it would raise  awareness about lingering PRs.
> >> Does
> >> >> anybody know if there is
> >> >> some way to integrate this into JIRA, so we can easily see (and
> >> >> filter/sort) lingering PRs?
> >> >>
> >> >> Cheers,
> >> >> Theo
> >> >>
> >> >> On Wed, Oct 7, 2015 at 12:04 PM, Vasiliki Kalavri <
> >> >> vasilikikalavri@gmail.com
> >> >> > wrote:
> >> >>
> >> >> > Hey,
> >> >> >
> >> >> > I agree that we need to organize the PR process. A PR management
> tool
> >> >> would
> >> >> > be great.
> >> >> >
> >> >> > However, it seems to me that the shepherding process described
is
> >> -more
> >> >> or
> >> >> > less- what we've already been doing. There is usually a person
that
> >> >> reviews
> >> >> > the PR and kind-of drives the process. Maybe making this explicit
> >> will
> >> >> make
> >> >> > things go faster.
> >> >> >
> >> >> > There is a problem I see here, quite related to what Theo also
> >> >> mentioned.
> >> >> > For how long do we let a PR lingering around without a shepherd?
> What
> >> >> do we
> >> >> > do if nobody volunteers?
> >> >> > Could we maybe do a "PR overall status assessment" once per week
or
> >> so,
> >> >> > where we find those problematic PRs and try to assign them / close
> >> them?
> >> >> >
> >> >> > Finally, I think shepherding one's own PR is a bad idea (the review
> >> >> part)
> >> >> > unless it's something trivial.
> >> >> >
> >> >> > Cheers and see you very soon,
> >> >> > -Vasia.
> >> >> > On Oct 7, 2015 11:24 AM, "Matthias J. Sax" <mjsax@apache.org>
> wrote:
> >> >> >
> >> >> > > Ok. That makes sense. So most people will need to change
behavior
> >> and
> >> >> > > start discussions in JIRA and not over dev list. Furthermore,
> >> issues
> >> >> > > list must be monitored more carefully... (I personally, watch
dev
> >> >> > > carefully and only skip over issues list right now)
> >> >> > >
> >> >> > > -Matthias
> >> >> > >
> >> >> > > On 10/07/2015 10:22 AM, Fabian Hueske wrote:
> >> >> > > > @Matthias: That's a good point. Each PR should be backed
by a
> >> JIRA
> >> >> > issue.
> >> >> > > > If that's not the case, we have to make the contributor
aware
> of
> >> >> that
> >> >> > > rule.
> >> >> > > > I'll update the process to keep all discussions in JIRA
(will
> be
> >> >> > mirrored
> >> >> > > > to issues ML), OK?
> >> >> > > >
> >> >> > > > @Theo: You are right. Adding this process won't be the
silver
> >> >> bullet to
> >> >> > > fix
> >> >> > > > all PR related issues.
> >> >> > > > But I hope it will help to improve the overall situation.
> >> >> > > >
> >> >> > > > Are there any other comment? Otherwise I would start
to prepare
> >> add
> >> >> > this
> >> >> > > to
> >> >> > > > our website.
> >> >> > > >
> >> >> > > > Thanks, Fabian
> >> >> > > >
> >> >> > > > 2015-10-06 9:46 GMT+02:00 Theodore Vasiloudis <
> >> >> > > > theodoros.vasiloudis@gmail.com>:
> >> >> > > >
> >> >> > > >> One problem that we are seeing with FlinkML PRs
is that there
> >> are
> >> >> > simply
> >> >> > > >> not enough commiters to "shepherd" all of them.
> >> >> > > >>
> >> >> > > >> While I think this process would help generally,
I don't think
> >> it
> >> >> > would
> >> >> > > >> solve this kind of problem.
> >> >> > > >>
> >> >> > > >> Regards,
> >> >> > > >> Theodore
> >> >> > > >>
> >> >> > > >> On Mon, Oct 5, 2015 at 3:28 PM, Matthias J. Sax
<
> >> mjsax@apache.org>
> >> >> > > wrote:
> >> >> > > >>
> >> >> > > >>> One comment:
> >> >> > > >>> We should ensure that contributors follow discussions
on the
> >> dev
> >> >> > > mailing
> >> >> > > >>> list. Otherwise, they might miss important discussions
> >> regarding
> >> >> > their
> >> >> > > >>> PR (what happened already). Thus, the contributor
was waiting
> >> for
> >> >> > > >>> feedback on the PR, while the reviewer(s) waited
for the PR
> to
> >> be
> >> >> > > >>> updated according to the discussion consensus,
resulting in
> >> >> > unnecessary
> >> >> > > >>> delays.
> >> >> > > >>>
> >> >> > > >>> -Matthias
> >> >> > > >>>
> >> >> > > >>>
> >> >> > > >>>
> >> >> > > >>> On 10/05/2015 02:18 PM, Fabian Hueske wrote:
> >> >> > > >>>> Hi everybody,
> >> >> > > >>>>
> >> >> > > >>>> Along with our efforts to improve the “How
to contribute”
> >> guide,
> >> >> I
> >> >> > > >> would
> >> >> > > >>>> like to start a discussion about a setting
up a review
> process
> >> >> for
> >> >> > > pull
> >> >> > > >>>> requests.
> >> >> > > >>>>
> >> >> > > >>>> Right now, I feel that our PR review efforts
are often a bit
> >> >> > > >> unorganized.
> >> >> > > >>>> This leads to situation such as:
> >> >> > > >>>>
> >> >> > > >>>> - PRs which are lingering around without
review or feedback
> >> >> > > >>>> - PRs which got a +1 for merging but which
did not get
> merged
> >> >> > > >>>> - PRs which have been rejected after a long
time
> >> >> > > >>>> - PRs which became irrelevant because some
component was
> >> >> rewritten
> >> >> > > >>>> - PRs which are lingering around and have
been abandoned by
> >> their
> >> >> > > >>>> contributors
> >> >> > > >>>>
> >> >> > > >>>> To address these issues I propose to define
a pull request
> >> review
> >> >> > > >> process
> >> >> > > >>>> as follows:
> >> >> > > >>>>
> >> >> > > >>>> 1. [Get a Shepherd] Each pull request is
taken care of by a
> >> >> > shepherd.
> >> >> > > >>>> Shepherds are committers that voluntarily
sign up and *feel
> >> >> > > >> responsible*
> >> >> > > >>>> for helping the PR through the process until
it is merged
> (or
> >> >> > > >> discarded).
> >> >> > > >>>> The shepherd is also the person to contact
for the author of
> >> the
> >> >> > PR. A
> >> >> > > >>>> committer becomes the shepherd of a PR by
dropping a comment
> >> on
> >> >> > Github
> >> >> > > >>> like
> >> >> > > >>>> “I would like to shepherd this PR”.
A PR can be reassigned
> >> with
> >> >> lazy
> >> >> > > >>>> consensus.
> >> >> > > >>>>
> >> >> > > >>>> 2. [Accept Decision] The first decision
for a PR should be
> >> >> whether
> >> >> > it
> >> >> > > >> is
> >> >> > > >>>> accepted or not. This depends on a) whether
it is a desired
> >> >> feature
> >> >> > or
> >> >> > > >>>> improvement for Flink and b) whether the
higher-level
> solution
> >> >> > design
> >> >> > > >> is
> >> >> > > >>>> appropriate. In many cases such as bug fixes
or discussed
> >> >> features
> >> >> > or
> >> >> > > >>>> improvements, this should be an easy decision.
In case of
> >> more a
> >> >> > > >> complex
> >> >> > > >>>> feature, the discussion should have been
started when the
> >> >> mandatory
> >> >> > > >> JIRA
> >> >> > > >>>> was created. If it is still not clear whether
the PR should
> be
> >> >> > > accepted
> >> >> > > >>> or
> >> >> > > >>>> not, a discussion should be started in JIRA
(a JIRA issue
> >> needs
> >> >> to
> >> >> > be
> >> >> > > >>>> created if none exists). The acceptance
decision should be
> >> >> recorded
> >> >> > by
> >> >> > > >> a
> >> >> > > >>>> “+1 to accept” message in Github. If
the PR is not accepted,
> >> it
> >> >> > should
> >> >> > > >> be
> >> >> > > >>>> closed in a timely manner.
> >> >> > > >>>>
> >> >> > > >>>> 3. [Merge Decision] Once a PR has been “accepted”,
it should
> >> be
> >> >> > > brought
> >> >> > > >>>> into a mergeable state. This means the community
should
> >> quickly
> >> >> > react
> >> >> > > >> on
> >> >> > > >>>> contributor questions or PR updates. Everybody
is encouraged
> >> to
> >> >> > review
> >> >> > > >>> pull
> >> >> > > >>>> requests and give feedback. Ideally, the
PR author does not
> >> have
> >> >> to
> >> >> > > >> wait
> >> >> > > >>>> for a long time to get feedback. The shepherd
of a PR should
> >> feel
> >> >> > > >>>> responsible to drive this process forward.
Once the PR is
> >> >> considered
> >> >> > > to
> >> >> > > >>> be
> >> >> > > >>>> mergeable, this should be recorded by a
“+1 to merge”
> message
> >> in
> >> >> > > >> Github.
> >> >> > > >>> If
> >> >> > > >>>> the pull request becomes abandoned at some
point in time, it
> >> >> should
> >> >> > be
> >> >> > > >>>> either taken over by somebody else or be
closed after a
> >> >> reasonable
> >> >> > > >> time.
> >> >> > > >>>> Again, this can be done by anybody, but
the shepherd should
> >> feel
> >> >> > > >>>> responsible to resolve the issue.
> >> >> > > >>>>
> >> >> > > >>>> 4. Once, the PR is in a mergeable state,
it should be
> merged.
> >> >> This
> >> >> > can
> >> >> > > >> be
> >> >> > > >>>> done by anybody, however the shepherd should
make sure that
> it
> >> >> > happens
> >> >> > > >>> in a
> >> >> > > >>>> timely manner.
> >> >> > > >>>>
> >> >> > > >>>> IMPORTANT: Everybody is encouraged to discuss
pull requests,
> >> give
> >> >> > > >>> feedback,
> >> >> > > >>>> and merge pull requests which are in a good
shape. However,
> >> the
> >> >> > > >> shepherd
> >> >> > > >>>> should feel responsible to drive a PR forward
if nothing
> >> happens.
> >> >> > > >>>>
> >> >> > > >>>> By defining a review process for pull requests,
I hope we
> can
> >> >> > > >>>>
> >> >> > > >>>> - Improve the satisfaction of and interaction
with
> >> contributors
> >> >> > > >>>> - Improve and speed-up the review process
of pull requests.
> >> >> > > >>>> - Close irrelevant and stale PRs more timely
> >> >> > > >>>> - Reduce the effort for code reviewing
> >> >> > > >>>>
> >> >> > > >>>> The review process can be supported by some
tooling:
> >> >> > > >>>>
> >> >> > > >>>> - A QA bot that checks the quality of pull
requests such as
> >> >> increase
> >> >> > > of
> >> >> > > >>>> compiler warnings, code style, API changes,
etc.
> >> >> > > >>>> - A PR management tool such as Sparks PR
dashboard (see:
> >> >> > > >>>> https://spark-prs.appspot.com/)
> >> >> > > >>>>
> >> >> > > >>>> I would like to start discussions about
using such tools
> >> later as
> >> >> > > >>> separate
> >> >> > > >>>> discussions.
> >> >> > > >>>>
> >> >> > > >>>> Looking forward to your feedback,
> >> >> > > >>>> Fabian
> >> >> > > >>>>
> >> >> > > >>>
> >> >> > > >>>
> >> >> > > >>
> >> >> > > >
> >> >> > >
> >> >> > >
> >> >> >
> >> >>
> >> >
> >> >
> >>
> >
> >
>

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