[DISCUSS] Introducing a review process for pull requests

[Fabian Hueske <fh...@gmail.com>]

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

Re: [DISCUSS] Introducing a review process for pull requests

[Márton Balassi <ba...@gmail.com>]

+1

One minor comment: I suppose you implicitly mean that a committer can
shepherd her own PR.

On Wed, Oct 7, 2015 at 10:22 AM, Fabian Hueske <fh...@gmail.com> 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 <mj...@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
> > > >
> > >
> > >
> >
>

Re: [DISCUSS] Introducing a review process for pull requests

[Chesnay Schepler <ch...@apache.org>]

I think we can't do that since we don't have direct control over the 
github repository, similar to how we have to rely on the bot to close PR's.

On 17.10.2015 12:32, Alexander Alexandrov wrote:
> 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 <fh...@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 <fh...@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" <mj...@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
>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>

Re: [DISCUSS] Introducing a review process for pull requests

[Henry Saputra <he...@gmail.com>]

+1 for that one.

The good news is all Flink committers have been very discipline about
reviews :)

On Monday, October 26, 2015, Fabian Hueske <fh...@gmail.com> wrote:

> Hi Matthias,
>
> those a good points. I did not really think about the different roles
> (technical, meta-role).
> My reasoning was that the shepherd should be able to give feedback on the
> PR in order to move it forward. This does not work so well if the shepherd
> is also the author of the PR (at least I am not so good at commenting on my
> own work ;-)).
>
> However, you and Henry are of course right. Every committer can commit by
> her/himself.
>
> How about we keep it as follows:
> Committers can of course push hot fixes and minor changes directly.
> Everything larger should go through a PR. If nobody signs up to shepherd
> the PR, the author can drive it forward her/himself. I don't think it is
> necessary that a committer explicitly sign-up as a shepherd of her/his own
> PR.
>
> Cheers, Fabian
>
>
> 2015-10-24 20:48 GMT+02:00 Henry Saputra <henry.saputra@gmail.com
> <javascript:;>>:
>
> > Yes, as committer we trust you to do the right thing when committing the
> > code.
> > If a committer believe he/ she needs review,especially with large
> > patch, then he/ she should send PR for review.
> >
> >
> > - Henry
> >
> > On Sat, Oct 24, 2015 at 3:53 AM, Matthias J. Sax <mjsax@apache.org
> <javascript:;>> wrote:
> > > 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
> <javascript:;>>:
> > >>
> > >>> 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 <javascript:;>>:
> > >>>
> > >>>> 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 <javascript:;>>:
> > >>>>
> > >>>>> 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
> <javascript:;>>:
> > >>>>>
> > >>>>>> 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
> <javascript:;>>:
> > >>>>>>
> > >>>>>>> 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 <javascript:;>>:
> > >>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> 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 <javascript:;>
> > >>>>>>>>> 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
> <javascript:;>>
> > >>>> 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 <javascript:;>>:
> > >>>>>>>>>>>
> > >>>>>>>>>>>> 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 <javascript:;>>
> > >>>>>>>>>> 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
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>>
> > >>
> > >
> >
>

Re: [DISCUSS] Introducing a review process for pull requests

["Matthias J. Sax" <mj...@apache.org>]

Sound good to me.

And I agree; I cannot comment on my own work either ;)

On 10/26/2015 05:01 PM, Fabian Hueske wrote:
> Hi Matthias,
> 
> those a good points. I did not really think about the different roles
> (technical, meta-role).
> My reasoning was that the shepherd should be able to give feedback on the
> PR in order to move it forward. This does not work so well if the shepherd
> is also the author of the PR (at least I am not so good at commenting on my
> own work ;-)).
> 
> However, you and Henry are of course right. Every committer can commit by
> her/himself.
> 
> How about we keep it as follows:
> Committers can of course push hot fixes and minor changes directly.
> Everything larger should go through a PR. If nobody signs up to shepherd
> the PR, the author can drive it forward her/himself. I don't think it is
> necessary that a committer explicitly sign-up as a shepherd of her/his own
> PR.
> 
> Cheers, Fabian
> 
> 
> 2015-10-24 20:48 GMT+02:00 Henry Saputra <he...@gmail.com>:
> 
>> Yes, as committer we trust you to do the right thing when committing the
>> code.
>> If a committer believe he/ she needs review,especially with large
>> patch, then he/ she should send PR for review.
>>
>>
>> - Henry
>>
>> On Sat, Oct 24, 2015 at 3:53 AM, Matthias J. Sax <mj...@apache.org> wrote:
>>> 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 <fh...@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 <fh...@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 <fh...@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" <mj...@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
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
> 

Re: [DISCUSS] Introducing a review process for pull requests

[Fabian Hueske <fh...@gmail.com>]

Hi Matthias,

those a good points. I did not really think about the different roles
(technical, meta-role).
My reasoning was that the shepherd should be able to give feedback on the
PR in order to move it forward. This does not work so well if the shepherd
is also the author of the PR (at least I am not so good at commenting on my
own work ;-)).

However, you and Henry are of course right. Every committer can commit by
her/himself.

How about we keep it as follows:
Committers can of course push hot fixes and minor changes directly.
Everything larger should go through a PR. If nobody signs up to shepherd
the PR, the author can drive it forward her/himself. I don't think it is
necessary that a committer explicitly sign-up as a shepherd of her/his own
PR.

Cheers, Fabian


2015-10-24 20:48 GMT+02:00 Henry Saputra <he...@gmail.com>:

> Yes, as committer we trust you to do the right thing when committing the
> code.
> If a committer believe he/ she needs review,especially with large
> patch, then he/ she should send PR for review.
>
>
> - Henry
>
> On Sat, Oct 24, 2015 at 3:53 AM, Matthias J. Sax <mj...@apache.org> wrote:
> > 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 <fh...@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 <fh...@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 <fh...@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" <mj...@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
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>
> >
>

Re: [DISCUSS] Introducing a review process for pull requests

[Henry Saputra <he...@gmail.com>]

Yes, as committer we trust you to do the right thing when committing the code.
If a committer believe he/ she needs review,especially with large
patch, then he/ she should send PR for review.


- Henry

On Sat, Oct 24, 2015 at 3:53 AM, Matthias J. Sax <mj...@apache.org> wrote:
> 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 <fh...@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 <fh...@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 <fh...@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" <mj...@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
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>

Re: [DISCUSS] Introducing a review process for pull requests

["Matthias J. Sax" <mj...@apache.org>]

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 <fh...@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 <fh...@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 <fh...@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" <mj...@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
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
> 

Re: [DISCUSS] Introducing a review process for pull requests

[Fabian Hueske <fh...@gmail.com>]

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 <fh...@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 <fh...@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 <fh...@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" <mj...@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
>> >> >> > > >>>>
>> >> >> > > >>>
>> >> >> > > >>>
>> >> >> > > >>
>> >> >> > > >
>> >> >> > >
>> >> >> > >
>> >> >> >
>> >> >>
>> >> >
>> >> >
>> >>
>> >
>> >
>>
>
>

Re: [DISCUSS] Introducing a review process for pull requests

[Fabian Hueske <fh...@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 <fh...@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 <fh...@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" <mj...@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
> >> >> > > >>>>
> >> >> > > >>>
> >> >> > > >>>
> >> >> > > >>
> >> >> > > >
> >> >> > >
> >> >> > >
> >> >> >
> >> >>
> >> >
> >> >
> >>
> >
> >
>

Re: [DISCUSS] Introducing a review process for pull requests

[Alexander Alexandrov <al...@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 <fh...@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 <fh...@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" <mj...@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
>> >> > > >>>>
>> >> > > >>>
>> >> > > >>>
>> >> > > >>
>> >> > > >
>> >> > >
>> >> > >
>> >> >
>> >>
>> >
>> >
>>
>
>

Re: [DISCUSS] Introducing a review process for pull requests

[Alexander Alexandrov <al...@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 <fh...@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 <fh...@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" <mj...@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
> >> > > >>>>
> >> > > >>>
> >> > > >>>
> >> > > >>
> >> > > >
> >> > >
> >> > >
> >> >
> >>
> >
> >
>

Re: [DISCUSS] Introducing a review process for pull requests

[Fabian Hueske <fh...@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 <fh...@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" <mj...@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 <mj...@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
>> > > >>>>
>> > > >>>
>> > > >>>
>> > > >>
>> > > >
>> > >
>> > >
>> >
>>
>
>

Re: [DISCUSS] Introducing a review process for pull requests

[Fabian Hueske <fh...@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" <mj...@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 <mj...@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
> > > >>>>
> > > >>>
> > > >>>
> > > >>
> > > >
> > >
> > >
> >
>

Re: [DISCUSS] Introducing a review process for pull requests

[Theodore Vasiloudis <th...@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" <mj...@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 <mj...@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
> > >>>>
> > >>>
> > >>>
> > >>
> > >
> >
> >
>

Re: [DISCUSS] Introducing a review process for pull requests

[Vasiliki Kalavri <va...@gmail.com>]

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" <mj...@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 <mj...@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
> >>>>
> >>>
> >>>
> >>
> >
>
>

Re: [DISCUSS] Introducing a review process for pull requests

["Matthias J. Sax" <mj...@apache.org>]

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 <mj...@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
>>>>
>>>
>>>
>>
> 

Re: [DISCUSS] Introducing a review process for pull requests

[Fabian Hueske <fh...@gmail.com>]

@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 <mj...@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
> > >
> >
> >
>

Re: [DISCUSS] Introducing a review process for pull requests

[Theodore Vasiloudis <th...@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 <mj...@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
> >
>
>

Re: [DISCUSS] Introducing a review process for pull requests

["Matthias J. Sax" <mj...@apache.org>]

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
>