flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthias J. Sax" <mj...@apache.org>
Subject Re: [DISCUSS] Introducing a review process for pull requests
Date Sat, 24 Oct 2015 10:53:16 GMT
Great work!

What puzzles me a little bit, is the shepherding of PR from committers.
Why should it be a different committer?

1) As a committer, you don't even need to open a PR for small code
changes at all (especially, if the committer is the most experience one
regard a certain component/library). Just open a JIRA, fix it, and commit.

2) It is clear, that if a PR is complex and maybe touches different
components, feedback from the most experiences committer on those
components should be given on the PR to ensure code quality. But the
role of a shepherd is a "meta" role, if a understand the guideline
correctly, and not a technical one (-> everybody should discuss,
comment, accept, mark to get merged, and merge PRs). So why do we need a
different shepherd there? I think, committers can drive their own PRs by
themselves.


-Matthias


On 10/23/2015 06:00 PM, Fabian Hueske wrote:
> Hi everybody,
> 
> I created a wiki page for our Pull Request Process. [1]
> Please review, refine, and comment.
> 
> I would suggest to start following the process once 0.10 is released.
> What do you think?
> 
> Cheers,
> Fabian
> 
> [1]
> https://cwiki.apache.org/confluence/display/FLINK/Pull+Request+Management
> 
> 2015-10-19 17:53 GMT+02:00 Fabian Hueske <fhueske@gmail.com>:
> 
>> 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
View raw message