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 Wed, 07 Oct 2015 14:57:55 GMT
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