[PROPOSAL] [community] A more structured approach to reviews and contributions

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[PROPOSAL] [community] A more structured approach to reviews and contributions

Stephan Ewen
Hi Flink community members!

As many of you will have noticed, the Flink project activity has gone up
again quite a bit.
There are many more contributions, which is an absolutely great thing to
have :-)

However, we see a continuously growing backlog of pull requests and JIRA
issues.
To make sure the community will be able to handle the increased volume, I
think we need to revisit some
approaches and processes. I believe there are a few opportunities to
structure things a bit better, which
should help to scale the development.

The first thing I would like to bring up are *Pull Request Reviews*. Even
though more community members being
active in reviews (which is a really great thing!) the Pull Request backlog
is increasing quite a bit.

Why are pull requests still not merged faster? Looking at the reviews, one
thing I noticed is that most reviews deal
immediately with detailed code issues, and leave out most of the core
questions that need to be answered
before a Pull Request can be merged, like "is this a desired feature?" or
"does this align well with other developments?".
I think that we even make things slightly worse that way: From my personal
experience, I have often thought "oh, this
PR has a review already" and rather looked at another PR, only to find
later that the first review did never decide whether
this PR is actually a good fit for Flink.

There has never been a proper documentation of how to answer these
questions, what to evaluate in reviews,
guidelines for how to evaluate pull requests, other than code quality. I
suspect that this is why so many reviewers
do not address the "is this a good contribution" questions, making pull
requests linger until another committers joins
the review.

Below is an idea for a guide *"How to Review Contributions"*. It outlines
five core aspects to be checked in every
pull request, and suggests a priority for clarifying those. The idea is
that this helps us to better structure reviews, and
to make each reviewer aware what we look for in a review and where to best
bring in their help.

Looking forward to comments!

Best,
Stephan

====================================

The draft is in this Google Doc. Please add small textual comments to the
doc, and bigger principle discussions as replies to this mail.

https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2cRbocGlGKCYnvJd9lVhk/edit?usp=sharing






















































*How to Review Contributions------------------------------This guide is for
all committers and contributors that want to help with reviewing
contributions. Thank you for your effort - good reviews are one the most
important and crucial parts of an open source project. This guide should
help the community to make reviews such that: - Contributors have a good
contribution experience- Reviews are structured and check all important
aspects of a contribution- Make sure we keep a high code quality in Flink-
We avoid situations where contributors and reviewers spend a lot of time to
refine a contribution that gets rejected laterReview ChecklistEvery review
needs to check the following five aspects. We encourage to check these
aspects in order, to avoid spending time on detailed code quality reviews
when there is not yet consensus that a feature or change should be actually
be added.(1) Is there consensus whether the change of feature should go
into to Flink?For bug fixes, this needs to be checked only in case it
requires bigger changes or might break existing programs and
setups.Ideally, this question is already answered from a JIRA issue or the
dev-list discussion, except in cases of bug fixes and small lightweight
additions/extensions. In that case, this question can be immediately marked
as resolved. For pull requests that are created without prior consensus,
this question needs to be answered as part of the review.The decision
whether the change should go into Flink needs to take the following aspects
into consideration: - Does the contribution alter the behavior of features
or components in a way that it may break previous users’ programs and
setups? If yes, there needs to be a discussion and agreement that this
change is desirable. - Does the contribution conceptually fit well into
Flink? Is it too much of special case such that it makes things more
complicated for the common case, or bloats the abstractions / APIs? - Does
the feature fit well into Flink’s architecture? Will it scale and keep
Flink flexible for the future, or will the feature restrict Flink in the
future? - Is the feature a significant new addition (rather than an
improvement to an existing part)? If yes, will the Flink community commit
to maintaining this feature? - Does the feature produce added value for
Flink users or developers? Or does it introduce risk of regression without
adding relevant user or developer benefit?All of these questions should be
answerable from the description/discussion in JIRA and Pull Request,
without looking at the code.(2) Does the contribution need attention from
some specific committers and is there time commitment from these
committers?Some changes require attention and approval from specific
committers. For example, changes in parts that are either very performance
sensitive, or have a critical impact on distributed coordination and fault
tolerance need input by a committer that is deeply familiar with the
component.As a rule of thumb, this is the case when the Pull Request
description answers one of the questions in the template section “Does this
pull request potentially affect one of the following parts” with ‘yes’.This
question can be answered with - Does not need specific attention- Needs
specific attention for X (X can be for example checkpointing, jobmanager,
etc.).- Has specific attention for X by @commiterA, @contributorBIf the
pull request needs specific attention, one of the tagged
committers/contributors should give the final approval.(3) Is the
contribution described well?Check whether the contribution is sufficiently
well described to support a good review. Trivial changes and fixes do not
need a long description. Any pull request that changes functionality or
behavior needs to describe the big picture of these changes, so that
reviews know what to look for (and don’t have to dig through the code to
hopefully understand what the change does).Changes that require longer
descriptions are ideally based on a prior design discussion in the mailing
list or in JIRA and can simply link to there or copy the description from
there.(4) Does the implementation follow the right overall
approach/architecture?Is this the best approach to implement the fix or
feature, or are there other approaches that would be easier, more robust,
or more maintainable?This question should be answerable from the Pull
Request description (or the linked JIRA) as much as possible.We recommend
to check this before diving into the details of commenting on individual
parts of the change.(5) Is the overall code quality good, meeting standard
we want to maintain in Flink?This is the detailed code review of the actual
changes, covering: - Are the changes doing what is described in the design
document or PR description?- Does the code follow the right software
engineering practices? It the code correct, robust, maintainable,
testable?- Are the change performance aware, when changing a performance
sensitive part?- Are the changes sufficiently covered by tests?- Are the
tests executing fast?- Does the code format follow Flink’s checkstyle
pattern?- Does the code avoid to introduce additional compiler
warnings?Some code style guidelines can be found in the [Flink Code Style
Page](https://flink.apache.org/contribute-code.html#code-style
<https://flink.apache.org/contribute-code.html#code-style>)Pull Request
Review TemplateAdd the following checklist to the pull request review,
checking the boxes as the questions are answered:  - [ ] Consensus that the
contribution should go into to Flink  - [ ] Does not need specific
attention | Needs specific attention for X | Has attention for X by Y  - [
] Contribution description  - [ ] Architectural approach  - [ ] Overall
code quality*
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

Till Rohrmann
Thanks for writing this up Stephan. I like the steps and hope that it will
help the community to make the review process better. Thus, +1 for putting
your proposal to practice.

Cheers,
Till

On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]> wrote:

> Hi Flink community members!
>
> As many of you will have noticed, the Flink project activity has gone up
> again quite a bit.
> There are many more contributions, which is an absolutely great thing to
> have :-)
>
> However, we see a continuously growing backlog of pull requests and JIRA
> issues.
> To make sure the community will be able to handle the increased volume, I
> think we need to revisit some
> approaches and processes. I believe there are a few opportunities to
> structure things a bit better, which
> should help to scale the development.
>
> The first thing I would like to bring up are *Pull Request Reviews*. Even
> though more community members being
> active in reviews (which is a really great thing!) the Pull Request backlog
> is increasing quite a bit.
>
> Why are pull requests still not merged faster? Looking at the reviews, one
> thing I noticed is that most reviews deal
> immediately with detailed code issues, and leave out most of the core
> questions that need to be answered
> before a Pull Request can be merged, like "is this a desired feature?" or
> "does this align well with other developments?".
> I think that we even make things slightly worse that way: From my personal
> experience, I have often thought "oh, this
> PR has a review already" and rather looked at another PR, only to find
> later that the first review did never decide whether
> this PR is actually a good fit for Flink.
>
> There has never been a proper documentation of how to answer these
> questions, what to evaluate in reviews,
> guidelines for how to evaluate pull requests, other than code quality. I
> suspect that this is why so many reviewers
> do not address the "is this a good contribution" questions, making pull
> requests linger until another committers joins
> the review.
>
> Below is an idea for a guide *"How to Review Contributions"*. It outlines
> five core aspects to be checked in every
> pull request, and suggests a priority for clarifying those. The idea is
> that this helps us to better structure reviews, and
> to make each reviewer aware what we look for in a review and where to best
> bring in their help.
>
> Looking forward to comments!
>
> Best,
> Stephan
>
> ====================================
>
> The draft is in this Google Doc. Please add small textual comments to the
> doc, and bigger principle discussions as replies to this mail.
>
>
> https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2cRbocGlGKCYnvJd9lVhk/edit?usp=sharing
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> *How to Review Contributions------------------------------This guide is for
> all committers and contributors that want to help with reviewing
> contributions. Thank you for your effort - good reviews are one the most
> important and crucial parts of an open source project. This guide should
> help the community to make reviews such that: - Contributors have a good
> contribution experience- Reviews are structured and check all important
> aspects of a contribution- Make sure we keep a high code quality in Flink-
> We avoid situations where contributors and reviewers spend a lot of time to
> refine a contribution that gets rejected laterReview ChecklistEvery review
> needs to check the following five aspects. We encourage to check these
> aspects in order, to avoid spending time on detailed code quality reviews
> when there is not yet consensus that a feature or change should be actually
> be added.(1) Is there consensus whether the change of feature should go
> into to Flink?For bug fixes, this needs to be checked only in case it
> requires bigger changes or might break existing programs and
> setups.Ideally, this question is already answered from a JIRA issue or the
> dev-list discussion, except in cases of bug fixes and small lightweight
> additions/extensions. In that case, this question can be immediately marked
> as resolved. For pull requests that are created without prior consensus,
> this question needs to be answered as part of the review.The decision
> whether the change should go into Flink needs to take the following aspects
> into consideration: - Does the contribution alter the behavior of features
> or components in a way that it may break previous users’ programs and
> setups? If yes, there needs to be a discussion and agreement that this
> change is desirable. - Does the contribution conceptually fit well into
> Flink? Is it too much of special case such that it makes things more
> complicated for the common case, or bloats the abstractions / APIs? - Does
> the feature fit well into Flink’s architecture? Will it scale and keep
> Flink flexible for the future, or will the feature restrict Flink in the
> future? - Is the feature a significant new addition (rather than an
> improvement to an existing part)? If yes, will the Flink community commit
> to maintaining this feature? - Does the feature produce added value for
> Flink users or developers? Or does it introduce risk of regression without
> adding relevant user or developer benefit?All of these questions should be
> answerable from the description/discussion in JIRA and Pull Request,
> without looking at the code.(2) Does the contribution need attention from
> some specific committers and is there time commitment from these
> committers?Some changes require attention and approval from specific
> committers. For example, changes in parts that are either very performance
> sensitive, or have a critical impact on distributed coordination and fault
> tolerance need input by a committer that is deeply familiar with the
> component.As a rule of thumb, this is the case when the Pull Request
> description answers one of the questions in the template section “Does this
> pull request potentially affect one of the following parts” with ‘yes’.This
> question can be answered with - Does not need specific attention- Needs
> specific attention for X (X can be for example checkpointing, jobmanager,
> etc.).- Has specific attention for X by @commiterA, @contributorBIf the
> pull request needs specific attention, one of the tagged
> committers/contributors should give the final approval.(3) Is the
> contribution described well?Check whether the contribution is sufficiently
> well described to support a good review. Trivial changes and fixes do not
> need a long description. Any pull request that changes functionality or
> behavior needs to describe the big picture of these changes, so that
> reviews know what to look for (and don’t have to dig through the code to
> hopefully understand what the change does).Changes that require longer
> descriptions are ideally based on a prior design discussion in the mailing
> list or in JIRA and can simply link to there or copy the description from
> there.(4) Does the implementation follow the right overall
> approach/architecture?Is this the best approach to implement the fix or
> feature, or are there other approaches that would be easier, more robust,
> or more maintainable?This question should be answerable from the Pull
> Request description (or the linked JIRA) as much as possible.We recommend
> to check this before diving into the details of commenting on individual
> parts of the change.(5) Is the overall code quality good, meeting standard
> we want to maintain in Flink?This is the detailed code review of the actual
> changes, covering: - Are the changes doing what is described in the design
> document or PR description?- Does the code follow the right software
> engineering practices? It the code correct, robust, maintainable,
> testable?- Are the change performance aware, when changing a performance
> sensitive part?- Are the changes sufficiently covered by tests?- Are the
> tests executing fast?- Does the code format follow Flink’s checkstyle
> pattern?- Does the code avoid to introduce additional compiler
> warnings?Some code style guidelines can be found in the [Flink Code Style
> Page](https://flink.apache.org/contribute-code.html#code-style
> <https://flink.apache.org/contribute-code.html#code-style>)Pull Request
> Review TemplateAdd the following checklist to the pull request review,
> checking the boxes as the questions are answered:  - [ ] Consensus that the
> contribution should go into to Flink  - [ ] Does not need specific
> attention | Needs specific attention for X | Has attention for X by Y  - [
> ] Contribution description  - [ ] Architectural approach  - [ ] Overall
> code quality*
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

tison
Hi Stephan,

Thanks for raising this discussion and the previous work! I
strongly support effort to improve the process of contributions and reviews.

As you mentioned above, Flink community responses  to contributions a bit
inefficiently, both for latency and quality. In my opinion the most
important things are to make sure reviews of contributions supervised and
push on the stage of a contribution step by step. The latter is similar as
you mentioned in the document.

Here are my observations and suggestions on these theme.

1. The document says that some contributions need need attention from some
specific committers. I definitely agree with that. In fact, under our
current process, every pull request is ideally named in format "[FLINK-XXX]
[module] Title". The module part indicates that in someways. However, there
is a time I make a contribution and know nothing about to whom I could cc.
Take a look at the Scala repo ( https://github.com/scala/scala/ ). It lists
by which committers a module maintain, I think it is really help for new
contributors.

2. I find that there are quite a number of pull requests of Flink are
inactive because no one comments or take concern of. Despite the reason it
is quite a bad experience. We surely reject some contributions but should
never disregard them. Here comes the idea that we always initially assign a
committer to a new contribution ( ideally, for we might have no so many
committer yet ). They are initially in charge of the contribution and take
the responsibility to nude it. Surely one can reassign to another who is
more properly, since the main issue here is ensure contribution supervised.

3. The document introduce five stages of a review, and take 2 into
consideration. I'd like to add 2 more stage, one of which is
"waiting-for-review" represents no one comments and "stall" represents pull
request cannot go ahead for some reason such as contributor doesn't
response, note that a "stall" pull requests is not rejected.
Having these previous, as Fabian Hueske comments in the document, we can
implement a bot to do such work. GitHub supports tagging pull requests
which meet our requirement of marking stage of a pr. Tags we would to
support include five stages in document, two addition stages as above.
Since a pr closed by rejected or merged and must with a conclusion in the
comments, we don't need that tag.
Rust-lang team implements a bot (
https://github.com/rust-lang-nursery/highfive ) which does these things
amazingly. See how https://github.com/rust-lang/rust/pulls works.

To sum up, I suggestion that we (1) introduce our committer and which
modules they maintain/familiar with at somewhere easy to find. (2) make
sure that every contribution is supervised. (3) Tag pull requests so that
the process more smoothly.

Best,
tison.


Till Rohrmann <[hidden email]> 于2018年9月17日周一 下午4:27写道:

> Thanks for writing this up Stephan. I like the steps and hope that it will
> help the community to make the review process better. Thus, +1 for putting
> your proposal to practice.
>
> Cheers,
> Till
>
> On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]> wrote:
>
> > Hi Flink community members!
> >
> > As many of you will have noticed, the Flink project activity has gone up
> > again quite a bit.
> > There are many more contributions, which is an absolutely great thing to
> > have :-)
> >
> > However, we see a continuously growing backlog of pull requests and JIRA
> > issues.
> > To make sure the community will be able to handle the increased volume, I
> > think we need to revisit some
> > approaches and processes. I believe there are a few opportunities to
> > structure things a bit better, which
> > should help to scale the development.
> >
> > The first thing I would like to bring up are *Pull Request Reviews*. Even
> > though more community members being
> > active in reviews (which is a really great thing!) the Pull Request
> backlog
> > is increasing quite a bit.
> >
> > Why are pull requests still not merged faster? Looking at the reviews,
> one
> > thing I noticed is that most reviews deal
> > immediately with detailed code issues, and leave out most of the core
> > questions that need to be answered
> > before a Pull Request can be merged, like "is this a desired feature?" or
> > "does this align well with other developments?".
> > I think that we even make things slightly worse that way: From my
> personal
> > experience, I have often thought "oh, this
> > PR has a review already" and rather looked at another PR, only to find
> > later that the first review did never decide whether
> > this PR is actually a good fit for Flink.
> >
> > There has never been a proper documentation of how to answer these
> > questions, what to evaluate in reviews,
> > guidelines for how to evaluate pull requests, other than code quality. I
> > suspect that this is why so many reviewers
> > do not address the "is this a good contribution" questions, making pull
> > requests linger until another committers joins
> > the review.
> >
> > Below is an idea for a guide *"How to Review Contributions"*. It outlines
> > five core aspects to be checked in every
> > pull request, and suggests a priority for clarifying those. The idea is
> > that this helps us to better structure reviews, and
> > to make each reviewer aware what we look for in a review and where to
> best
> > bring in their help.
> >
> > Looking forward to comments!
> >
> > Best,
> > Stephan
> >
> > ====================================
> >
> > The draft is in this Google Doc. Please add small textual comments to the
> > doc, and bigger principle discussions as replies to this mail.
> >
> >
> >
> https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2cRbocGlGKCYnvJd9lVhk/edit?usp=sharing
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > *How to Review Contributions------------------------------This guide is
> for
> > all committers and contributors that want to help with reviewing
> > contributions. Thank you for your effort - good reviews are one the most
> > important and crucial parts of an open source project. This guide should
> > help the community to make reviews such that: - Contributors have a good
> > contribution experience- Reviews are structured and check all important
> > aspects of a contribution- Make sure we keep a high code quality in
> Flink-
> > We avoid situations where contributors and reviewers spend a lot of time
> to
> > refine a contribution that gets rejected laterReview ChecklistEvery
> review
> > needs to check the following five aspects. We encourage to check these
> > aspects in order, to avoid spending time on detailed code quality reviews
> > when there is not yet consensus that a feature or change should be
> actually
> > be added.(1) Is there consensus whether the change of feature should go
> > into to Flink?For bug fixes, this needs to be checked only in case it
> > requires bigger changes or might break existing programs and
> > setups.Ideally, this question is already answered from a JIRA issue or
> the
> > dev-list discussion, except in cases of bug fixes and small lightweight
> > additions/extensions. In that case, this question can be immediately
> marked
> > as resolved. For pull requests that are created without prior consensus,
> > this question needs to be answered as part of the review.The decision
> > whether the change should go into Flink needs to take the following
> aspects
> > into consideration: - Does the contribution alter the behavior of
> features
> > or components in a way that it may break previous users’ programs and
> > setups? If yes, there needs to be a discussion and agreement that this
> > change is desirable. - Does the contribution conceptually fit well into
> > Flink? Is it too much of special case such that it makes things more
> > complicated for the common case, or bloats the abstractions / APIs? -
> Does
> > the feature fit well into Flink’s architecture? Will it scale and keep
> > Flink flexible for the future, or will the feature restrict Flink in the
> > future? - Is the feature a significant new addition (rather than an
> > improvement to an existing part)? If yes, will the Flink community commit
> > to maintaining this feature? - Does the feature produce added value for
> > Flink users or developers? Or does it introduce risk of regression
> without
> > adding relevant user or developer benefit?All of these questions should
> be
> > answerable from the description/discussion in JIRA and Pull Request,
> > without looking at the code.(2) Does the contribution need attention from
> > some specific committers and is there time commitment from these
> > committers?Some changes require attention and approval from specific
> > committers. For example, changes in parts that are either very
> performance
> > sensitive, or have a critical impact on distributed coordination and
> fault
> > tolerance need input by a committer that is deeply familiar with the
> > component.As a rule of thumb, this is the case when the Pull Request
> > description answers one of the questions in the template section “Does
> this
> > pull request potentially affect one of the following parts” with
> ‘yes’.This
> > question can be answered with - Does not need specific attention- Needs
> > specific attention for X (X can be for example checkpointing, jobmanager,
> > etc.).- Has specific attention for X by @commiterA, @contributorBIf the
> > pull request needs specific attention, one of the tagged
> > committers/contributors should give the final approval.(3) Is the
> > contribution described well?Check whether the contribution is
> sufficiently
> > well described to support a good review. Trivial changes and fixes do not
> > need a long description. Any pull request that changes functionality or
> > behavior needs to describe the big picture of these changes, so that
> > reviews know what to look for (and don’t have to dig through the code to
> > hopefully understand what the change does).Changes that require longer
> > descriptions are ideally based on a prior design discussion in the
> mailing
> > list or in JIRA and can simply link to there or copy the description from
> > there.(4) Does the implementation follow the right overall
> > approach/architecture?Is this the best approach to implement the fix or
> > feature, or are there other approaches that would be easier, more robust,
> > or more maintainable?This question should be answerable from the Pull
> > Request description (or the linked JIRA) as much as possible.We recommend
> > to check this before diving into the details of commenting on individual
> > parts of the change.(5) Is the overall code quality good, meeting
> standard
> > we want to maintain in Flink?This is the detailed code review of the
> actual
> > changes, covering: - Are the changes doing what is described in the
> design
> > document or PR description?- Does the code follow the right software
> > engineering practices? It the code correct, robust, maintainable,
> > testable?- Are the change performance aware, when changing a performance
> > sensitive part?- Are the changes sufficiently covered by tests?- Are the
> > tests executing fast?- Does the code format follow Flink’s checkstyle
> > pattern?- Does the code avoid to introduce additional compiler
> > warnings?Some code style guidelines can be found in the [Flink Code Style
> > Page](https://flink.apache.org/contribute-code.html#code-style
> > <https://flink.apache.org/contribute-code.html#code-style>)Pull Request
> > Review TemplateAdd the following checklist to the pull request review,
> > checking the boxes as the questions are answered:  - [ ] Consensus that
> the
> > contribution should go into to Flink  - [ ] Does not need specific
> > attention | Needs specific attention for X | Has attention for X by Y  -
> [
> > ] Contribution description  - [ ] Architectural approach  - [ ] Overall
> > code quality*
> >
>
Reply | Threaded
Open this post in threaded view
|

回复:[PROPOSAL] [community] A more structured approach to reviews and contributions

Zhijiang(wangzhijiang999)
In reply to this post by Till Rohrmann
From my personal experience as a contributor for three years, I feel better experience in contirbuting or reviewing than before, although we still have some points for further progress.

I reviewed the proposal doc, and it gives very constructive and meaningful guides which could help both contributor and reviewer. I agree with the bove suggestions and wish they can be praticed well!

Best,
Zhijiang
------------------------------------------------------------------
发件人:Till Rohrmann <[hidden email]>
发送时间:2018年9月17日(星期一) 16:27
收件人:dev <[hidden email]>
主 题:Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

Thanks for writing this up Stephan. I like the steps and hope that it will
help the community to make the review process better. Thus, +1 for putting
your proposal to practice.

Cheers,
Till

On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]> wrote:

> Hi Flink community members!
>
> As many of you will have noticed, the Flink project activity has gone up
> again quite a bit.
> There are many more contributions, which is an absolutely great thing to
> have :-)
>
> However, we see a continuously growing backlog of pull requests and JIRA
> issues.
> To make sure the community will be able to handle the increased volume, I
> think we need to revisit some
> approaches and processes. I believe there are a few opportunities to
> structure things a bit better, which
> should help to scale the development.
>
> The first thing I would like to bring up are *Pull Request Reviews*. Even
> though more community members being
> active in reviews (which is a really great thing!) the Pull Request backlog
> is increasing quite a bit.
>
> Why are pull requests still not merged faster? Looking at the reviews, one
> thing I noticed is that most reviews deal
> immediately with detailed code issues, and leave out most of the core
> questions that need to be answered
> before a Pull Request can be merged, like "is this a desired feature?" or
> "does this align well with other developments?".
> I think that we even make things slightly worse that way: From my personal
> experience, I have often thought "oh, this
> PR has a review already" and rather looked at another PR, only to find
> later that the first review did never decide whether
> this PR is actually a good fit for Flink.
>
> There has never been a proper documentation of how to answer these
> questions, what to evaluate in reviews,
> guidelines for how to evaluate pull requests, other than code quality. I
> suspect that this is why so many reviewers
> do not address the "is this a good contribution" questions, making pull
> requests linger until another committers joins
> the review.
>
> Below is an idea for a guide *"How to Review Contributions"*. It outlines
> five core aspects to be checked in every
> pull request, and suggests a priority for clarifying those. The idea is
> that this helps us to better structure reviews, and
> to make each reviewer aware what we look for in a review and where to best
> bring in their help.
>
> Looking forward to comments!
>
> Best,
> Stephan
>
> ====================================
>
> The draft is in this Google Doc. Please add small textual comments to the
> doc, and bigger principle discussions as replies to this mail.
>
>
> https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2cRbocGlGKCYnvJd9lVhk/edit?usp=sharing
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> *How to Review Contributions------------------------------This guide is for
> all committers and contributors that want to help with reviewing
> contributions. Thank you for your effort - good reviews are one the most
> important and crucial parts of an open source project. This guide should
> help the community to make reviews such that: - Contributors have a good
> contribution experience- Reviews are structured and check all important
> aspects of a contribution- Make sure we keep a high code quality in Flink-
> We avoid situations where contributors and reviewers spend a lot of time to
> refine a contribution that gets rejected laterReview ChecklistEvery review
> needs to check the following five aspects. We encourage to check these
> aspects in order, to avoid spending time on detailed code quality reviews
> when there is not yet consensus that a feature or change should be actually
> be added.(1) Is there consensus whether the change of feature should go
> into to Flink?For bug fixes, this needs to be checked only in case it
> requires bigger changes or might break existing programs and
> setups.Ideally, this question is already answered from a JIRA issue or the
> dev-list discussion, except in cases of bug fixes and small lightweight
> additions/extensions. In that case, this question can be immediately marked
> as resolved. For pull requests that are created without prior consensus,
> this question needs to be answered as part of the review.The decision
> whether the change should go into Flink needs to take the following aspects
> into consideration: - Does the contribution alter the behavior of features
> or components in a way that it may break previous users’ programs and
> setups? If yes, there needs to be a discussion and agreement that this
> change is desirable. - Does the contribution conceptually fit well into
> Flink? Is it too much of special case such that it makes things more
> complicated for the common case, or bloats the abstractions / APIs? - Does
> the feature fit well into Flink’s architecture? Will it scale and keep
> Flink flexible for the future, or will the feature restrict Flink in the
> future? - Is the feature a significant new addition (rather than an
> improvement to an existing part)? If yes, will the Flink community commit
> to maintaining this feature? - Does the feature produce added value for
> Flink users or developers? Or does it introduce risk of regression without
> adding relevant user or developer benefit?All of these questions should be
> answerable from the description/discussion in JIRA and Pull Request,
> without looking at the code.(2) Does the contribution need attention from
> some specific committers and is there time commitment from these
> committers?Some changes require attention and approval from specific
> committers. For example, changes in parts that are either very performance
> sensitive, or have a critical impact on distributed coordination and fault
> tolerance need input by a committer that is deeply familiar with the
> component.As a rule of thumb, this is the case when the Pull Request
> description answers one of the questions in the template section “Does this
> pull request potentially affect one of the following parts” with ‘yes’.This
> question can be answered with - Does not need specific attention- Needs
> specific attention for X (X can be for example checkpointing, jobmanager,
> etc.).- Has specific attention for X by @commiterA, @contributorBIf the
> pull request needs specific attention, one of the tagged
> committers/contributors should give the final approval.(3) Is the
> contribution described well?Check whether the contribution is sufficiently
> well described to support a good review. Trivial changes and fixes do not
> need a long description. Any pull request that changes functionality or
> behavior needs to describe the big picture of these changes, so that
> reviews know what to look for (and don’t have to dig through the code to
> hopefully understand what the change does).Changes that require longer
> descriptions are ideally based on a prior design discussion in the mailing
> list or in JIRA and can simply link to there or copy the description from
> there.(4) Does the implementation follow the right overall
> approach/architecture?Is this the best approach to implement the fix or
> feature, or are there other approaches that would be easier, more robust,
> or more maintainable?This question should be answerable from the Pull
> Request description (or the linked JIRA) as much as possible.We recommend
> to check this before diving into the details of commenting on individual
> parts of the change.(5) Is the overall code quality good, meeting standard
> we want to maintain in Flink?This is the detailed code review of the actual
> changes, covering: - Are the changes doing what is described in the design
> document or PR description?- Does the code follow the right software
> engineering practices? It the code correct, robust, maintainable,
> testable?- Are the change performance aware, when changing a performance
> sensitive part?- Are the changes sufficiently covered by tests?- Are the
> tests executing fast?- Does the code format follow Flink’s checkstyle
> pattern?- Does the code avoid to introduce additional compiler
> warnings?Some code style guidelines can be found in the [Flink Code Style
> Page](https://flink.apache.org/contribute-code.html#code-style
> <https://flink.apache.org/contribute-code.html#code-style>)Pull Request
> Review TemplateAdd the following checklist to the pull request review,
> checking the boxes as the questions are answered:  - [ ] Consensus that the
> contribution should go into to Flink  - [ ] Does not need specific
> attention | Needs specific attention for X | Has attention for X by Y  - [
> ] Contribution description  - [ ] Architectural approach  - [ ] Overall
> code quality*
>

Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

Stephan Ewen
Hi!

Thanks you for the encouraging feedback so far.

The overall goal is definitely to make the contribution process better and
get fewer pull requests that are disregarded.

There are various reasons for the disregarded pull requests, one being that
fewer committers really participate in reviews beyond
the component they are currently very involved with. This is a separate
issue and I am thinking on how to encourage more
activity there.

The other reason I was lack of structure and lack of decision making, which
is what I am first trying to fix here.
A follow-up to this will definitely be to improve the contribution guide as
well.

Best,
Stephan


On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
[hidden email]> wrote:

> From my personal experience as a contributor for three years, I feel
> better experience in contirbuting or reviewing than before, although we
> still have some points for further progress.
>
> I reviewed the proposal doc, and it gives very constructive and meaningful
> guides which could help both contributor and reviewer. I agree with the
> bove suggestions and wish they can be praticed well!
>
> Best,
> Zhijiang
> ------------------------------------------------------------------
> 发件人:Till Rohrmann <[hidden email]>
> 发送时间:2018年9月17日(星期一) 16:27
> 收件人:dev <[hidden email]>
> 主 题:Re: [PROPOSAL] [community] A more structured approach to reviews and
> contributions
>
> Thanks for writing this up Stephan. I like the steps and hope that it will
> help the community to make the review process better. Thus, +1 for putting
> your proposal to practice.
>
> Cheers,
> Till
>
> On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]> wrote:
>
> > Hi Flink community members!
> >
> > As many of you will have noticed, the Flink project activity has gone up
> > again quite a bit.
> > There are many more contributions, which is an absolutely great thing to
> > have :-)
> >
> > However, we see a continuously growing backlog of pull requests and JIRA
> > issues.
> > To make sure the community will be able to handle the increased volume, I
> > think we need to revisit some
> > approaches and processes. I believe there are a few opportunities to
> > structure things a bit better, which
> > should help to scale the development.
> >
> > The first thing I would like to bring up are *Pull Request Reviews*. Even
> > though more community members being
> > active in reviews (which is a really great thing!) the Pull Request
> backlog
> > is increasing quite a bit.
> >
> > Why are pull requests still not merged faster? Looking at the reviews,
> one
> > thing I noticed is that most reviews deal
> > immediately with detailed code issues, and leave out most of the core
> > questions that need to be answered
> > before a Pull Request can be merged, like "is this a desired feature?" or
> > "does this align well with other developments?".
> > I think that we even make things slightly worse that way: From my
> personal
> > experience, I have often thought "oh, this
> > PR has a review already" and rather looked at another PR, only to find
> > later that the first review did never decide whether
> > this PR is actually a good fit for Flink.
> >
> > There has never been a proper documentation of how to answer these
> > questions, what to evaluate in reviews,
> > guidelines for how to evaluate pull requests, other than code quality. I
> > suspect that this is why so many reviewers
> > do not address the "is this a good contribution" questions, making pull
> > requests linger until another committers joins
> > the review.
> >
> > Below is an idea for a guide *"How to Review Contributions"*. It outlines
> > five core aspects to be checked in every
> > pull request, and suggests a priority for clarifying those. The idea is
> > that this helps us to better structure reviews, and
> > to make each reviewer aware what we look for in a review and where to
> best
> > bring in their help.
> >
> > Looking forward to comments!
> >
> > Best,
> > Stephan
> >
> > ====================================
> >
> > The draft is in this Google Doc. Please add small textual comments to the
> > doc, and bigger principle discussions as replies to this mail.
> >
> >
> > https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
> RbocGlGKCYnvJd9lVhk/edit?usp=sharing
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > *How to Review Contributions------------------------------This guide is
> for
> > all committers and contributors that want to help with reviewing
> > contributions. Thank you for your effort - good reviews are one the most
> > important and crucial parts of an open source project. This guide should
> > help the community to make reviews such that: - Contributors have a good
> > contribution experience- Reviews are structured and check all important
> > aspects of a contribution- Make sure we keep a high code quality in
> Flink-
> > We avoid situations where contributors and reviewers spend a lot of time
> to
> > refine a contribution that gets rejected laterReview ChecklistEvery
> review
> > needs to check the following five aspects. We encourage to check these
> > aspects in order, to avoid spending time on detailed code quality reviews
> > when there is not yet consensus that a feature or change should be
> actually
> > be added.(1) Is there consensus whether the change of feature should go
> > into to Flink?For bug fixes, this needs to be checked only in case it
> > requires bigger changes or might break existing programs and
> > setups.Ideally, this question is already answered from a JIRA issue or
> the
> > dev-list discussion, except in cases of bug fixes and small lightweight
> > additions/extensions. In that case, this question can be immediately
> marked
> > as resolved. For pull requests that are created without prior consensus,
> > this question needs to be answered as part of the review.The decision
> > whether the change should go into Flink needs to take the following
> aspects
> > into consideration: - Does the contribution alter the behavior of
> features
> > or components in a way that it may break previous users’ programs and
> > setups? If yes, there needs to be a discussion and agreement that this
> > change is desirable. - Does the contribution conceptually fit well into
> > Flink? Is it too much of special case such that it makes things more
> > complicated for the common case, or bloats the abstractions / APIs? -
> Does
> > the feature fit well into Flink’s architecture? Will it scale and keep
> > Flink flexible for the future, or will the feature restrict Flink in the
> > future? - Is the feature a significant new addition (rather than an
> > improvement to an existing part)? If yes, will the Flink community commit
> > to maintaining this feature? - Does the feature produce added value for
> > Flink users or developers? Or does it introduce risk of regression
> without
> > adding relevant user or developer benefit?All of these questions should
> be
> > answerable from the description/discussion in JIRA and Pull Request,
> > without looking at the code.(2) Does the contribution need attention from
> > some specific committers and is there time commitment from these
> > committers?Some changes require attention and approval from specific
> > committers. For example, changes in parts that are either very
> performance
> > sensitive, or have a critical impact on distributed coordination and
> fault
> > tolerance need input by a committer that is deeply familiar with the
> > component.As a rule of thumb, this is the case when the Pull Request
> > description answers one of the questions in the template section “Does
> this
> > pull request potentially affect one of the following parts” with
> ‘yes’.This
> > question can be answered with - Does not need specific attention- Needs
> > specific attention for X (X can be for example checkpointing, jobmanager,
> > etc.).- Has specific attention for X by @commiterA, @contributorBIf the
> > pull request needs specific attention, one of the tagged
> > committers/contributors should give the final approval.(3) Is the
> > contribution described well?Check whether the contribution is
> sufficiently
> > well described to support a good review. Trivial changes and fixes do not
> > need a long description. Any pull request that changes functionality or
> > behavior needs to describe the big picture of these changes, so that
> > reviews know what to look for (and don’t have to dig through the code to
> > hopefully understand what the change does).Changes that require longer
> > descriptions are ideally based on a prior design discussion in the
> mailing
> > list or in JIRA and can simply link to there or copy the description from
> > there.(4) Does the implementation follow the right overall
> > approach/architecture?Is this the best approach to implement the fix or
> > feature, or are there other approaches that would be easier, more robust,
> > or more maintainable?This question should be answerable from the Pull
> > Request description (or the linked JIRA) as much as possible.We recommend
> > to check this before diving into the details of commenting on individual
> > parts of the change.(5) Is the overall code quality good, meeting
> standard
> > we want to maintain in Flink?This is the detailed code review of the
> actual
> > changes, covering: - Are the changes doing what is described in the
> design
> > document or PR description?- Does the code follow the right software
> > engineering practices? It the code correct, robust, maintainable,
> > testable?- Are the change performance aware, when changing a performance
> > sensitive part?- Are the changes sufficiently covered by tests?- Are the
> > tests executing fast?- Does the code format follow Flink’s checkstyle
> > pattern?- Does the code avoid to introduce additional compiler
> > warnings?Some code style guidelines can be found in the [Flink Code Style
> > Page](https://flink.apache.org/contribute-code.html#code-style
> > <https://flink.apache.org/contribute-code.html#code-style>)Pull Request
> > Review TemplateAdd the following checklist to the pull request review,
> > checking the boxes as the questions are answered:  - [ ] Consensus that
> the
> > contribution should go into to Flink  - [ ] Does not need specific
> > attention | Needs specific attention for X | Has attention for X by Y  -
> [
> > ] Contribution description  - [ ] Architectural approach  - [ ] Overall
> > code quality*
> >
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

Rong Rong
Thanks for putting the review contribution doc together, Stephan! This will
definitely help the community to make the review process better.

From my experience this will benefit on both contributors and reviewers
side! Thus +1 for putting into practice as well.

--
Rong

On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]> wrote:

> Hi!
>
> Thanks you for the encouraging feedback so far.
>
> The overall goal is definitely to make the contribution process better and
> get fewer pull requests that are disregarded.
>
> There are various reasons for the disregarded pull requests, one being that
> fewer committers really participate in reviews beyond
> the component they are currently very involved with. This is a separate
> issue and I am thinking on how to encourage more
> activity there.
>
> The other reason I was lack of structure and lack of decision making, which
> is what I am first trying to fix here.
> A follow-up to this will definitely be to improve the contribution guide as
> well.
>
> Best,
> Stephan
>
>
> On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
> [hidden email]> wrote:
>
> > From my personal experience as a contributor for three years, I feel
> > better experience in contirbuting or reviewing than before, although we
> > still have some points for further progress.
> >
> > I reviewed the proposal doc, and it gives very constructive and
> meaningful
> > guides which could help both contributor and reviewer. I agree with the
> > bove suggestions and wish they can be praticed well!
> >
> > Best,
> > Zhijiang
> > ------------------------------------------------------------------
> > 发件人:Till Rohrmann <[hidden email]>
> > 发送时间:2018年9月17日(星期一) 16:27
> > 收件人:dev <[hidden email]>
> > 主 题:Re: [PROPOSAL] [community] A more structured approach to reviews and
> > contributions
> >
> > Thanks for writing this up Stephan. I like the steps and hope that it
> will
> > help the community to make the review process better. Thus, +1 for
> putting
> > your proposal to practice.
> >
> > Cheers,
> > Till
> >
> > On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]> wrote:
> >
> > > Hi Flink community members!
> > >
> > > As many of you will have noticed, the Flink project activity has gone
> up
> > > again quite a bit.
> > > There are many more contributions, which is an absolutely great thing
> to
> > > have :-)
> > >
> > > However, we see a continuously growing backlog of pull requests and
> JIRA
> > > issues.
> > > To make sure the community will be able to handle the increased
> volume, I
> > > think we need to revisit some
> > > approaches and processes. I believe there are a few opportunities to
> > > structure things a bit better, which
> > > should help to scale the development.
> > >
> > > The first thing I would like to bring up are *Pull Request Reviews*.
> Even
> > > though more community members being
> > > active in reviews (which is a really great thing!) the Pull Request
> > backlog
> > > is increasing quite a bit.
> > >
> > > Why are pull requests still not merged faster? Looking at the reviews,
> > one
> > > thing I noticed is that most reviews deal
> > > immediately with detailed code issues, and leave out most of the core
> > > questions that need to be answered
> > > before a Pull Request can be merged, like "is this a desired feature?"
> or
> > > "does this align well with other developments?".
> > > I think that we even make things slightly worse that way: From my
> > personal
> > > experience, I have often thought "oh, this
> > > PR has a review already" and rather looked at another PR, only to find
> > > later that the first review did never decide whether
> > > this PR is actually a good fit for Flink.
> > >
> > > There has never been a proper documentation of how to answer these
> > > questions, what to evaluate in reviews,
> > > guidelines for how to evaluate pull requests, other than code quality.
> I
> > > suspect that this is why so many reviewers
> > > do not address the "is this a good contribution" questions, making pull
> > > requests linger until another committers joins
> > > the review.
> > >
> > > Below is an idea for a guide *"How to Review Contributions"*. It
> outlines
> > > five core aspects to be checked in every
> > > pull request, and suggests a priority for clarifying those. The idea is
> > > that this helps us to better structure reviews, and
> > > to make each reviewer aware what we look for in a review and where to
> > best
> > > bring in their help.
> > >
> > > Looking forward to comments!
> > >
> > > Best,
> > > Stephan
> > >
> > > ====================================
> > >
> > > The draft is in this Google Doc. Please add small textual comments to
> the
> > > doc, and bigger principle discussions as replies to this mail.
> > >
> > >
> > > https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
> > RbocGlGKCYnvJd9lVhk/edit?usp=sharing
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > *How to Review Contributions------------------------------This guide is
> > for
> > > all committers and contributors that want to help with reviewing
> > > contributions. Thank you for your effort - good reviews are one the
> most
> > > important and crucial parts of an open source project. This guide
> should
> > > help the community to make reviews such that: - Contributors have a
> good
> > > contribution experience- Reviews are structured and check all important
> > > aspects of a contribution- Make sure we keep a high code quality in
> > Flink-
> > > We avoid situations where contributors and reviewers spend a lot of
> time
> > to
> > > refine a contribution that gets rejected laterReview ChecklistEvery
> > review
> > > needs to check the following five aspects. We encourage to check these
> > > aspects in order, to avoid spending time on detailed code quality
> reviews
> > > when there is not yet consensus that a feature or change should be
> > actually
> > > be added.(1) Is there consensus whether the change of feature should go
> > > into to Flink?For bug fixes, this needs to be checked only in case it
> > > requires bigger changes or might break existing programs and
> > > setups.Ideally, this question is already answered from a JIRA issue or
> > the
> > > dev-list discussion, except in cases of bug fixes and small lightweight
> > > additions/extensions. In that case, this question can be immediately
> > marked
> > > as resolved. For pull requests that are created without prior
> consensus,
> > > this question needs to be answered as part of the review.The decision
> > > whether the change should go into Flink needs to take the following
> > aspects
> > > into consideration: - Does the contribution alter the behavior of
> > features
> > > or components in a way that it may break previous users’ programs and
> > > setups? If yes, there needs to be a discussion and agreement that this
> > > change is desirable. - Does the contribution conceptually fit well into
> > > Flink? Is it too much of special case such that it makes things more
> > > complicated for the common case, or bloats the abstractions / APIs? -
> > Does
> > > the feature fit well into Flink’s architecture? Will it scale and keep
> > > Flink flexible for the future, or will the feature restrict Flink in
> the
> > > future? - Is the feature a significant new addition (rather than an
> > > improvement to an existing part)? If yes, will the Flink community
> commit
> > > to maintaining this feature? - Does the feature produce added value for
> > > Flink users or developers? Or does it introduce risk of regression
> > without
> > > adding relevant user or developer benefit?All of these questions should
> > be
> > > answerable from the description/discussion in JIRA and Pull Request,
> > > without looking at the code.(2) Does the contribution need attention
> from
> > > some specific committers and is there time commitment from these
> > > committers?Some changes require attention and approval from specific
> > > committers. For example, changes in parts that are either very
> > performance
> > > sensitive, or have a critical impact on distributed coordination and
> > fault
> > > tolerance need input by a committer that is deeply familiar with the
> > > component.As a rule of thumb, this is the case when the Pull Request
> > > description answers one of the questions in the template section “Does
> > this
> > > pull request potentially affect one of the following parts” with
> > ‘yes’.This
> > > question can be answered with - Does not need specific attention- Needs
> > > specific attention for X (X can be for example checkpointing,
> jobmanager,
> > > etc.).- Has specific attention for X by @commiterA, @contributorBIf the
> > > pull request needs specific attention, one of the tagged
> > > committers/contributors should give the final approval.(3) Is the
> > > contribution described well?Check whether the contribution is
> > sufficiently
> > > well described to support a good review. Trivial changes and fixes do
> not
> > > need a long description. Any pull request that changes functionality or
> > > behavior needs to describe the big picture of these changes, so that
> > > reviews know what to look for (and don’t have to dig through the code
> to
> > > hopefully understand what the change does).Changes that require longer
> > > descriptions are ideally based on a prior design discussion in the
> > mailing
> > > list or in JIRA and can simply link to there or copy the description
> from
> > > there.(4) Does the implementation follow the right overall
> > > approach/architecture?Is this the best approach to implement the fix or
> > > feature, or are there other approaches that would be easier, more
> robust,
> > > or more maintainable?This question should be answerable from the Pull
> > > Request description (or the linked JIRA) as much as possible.We
> recommend
> > > to check this before diving into the details of commenting on
> individual
> > > parts of the change.(5) Is the overall code quality good, meeting
> > standard
> > > we want to maintain in Flink?This is the detailed code review of the
> > actual
> > > changes, covering: - Are the changes doing what is described in the
> > design
> > > document or PR description?- Does the code follow the right software
> > > engineering practices? It the code correct, robust, maintainable,
> > > testable?- Are the change performance aware, when changing a
> performance
> > > sensitive part?- Are the changes sufficiently covered by tests?- Are
> the
> > > tests executing fast?- Does the code format follow Flink’s checkstyle
> > > pattern?- Does the code avoid to introduce additional compiler
> > > warnings?Some code style guidelines can be found in the [Flink Code
> Style
> > > Page](https://flink.apache.org/contribute-code.html#code-style
> > > <https://flink.apache.org/contribute-code.html#code-style>)Pull
> Request
> > > Review TemplateAdd the following checklist to the pull request review,
> > > checking the boxes as the questions are answered:  - [ ] Consensus that
> > the
> > > contribution should go into to Flink  - [ ] Does not need specific
> > > attention | Needs specific attention for X | Has attention for X by Y
> -
> > [
> > > ] Contribution description  - [ ] Architectural approach  - [ ] Overall
> > > code quality*
> > >
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

tison
Hi all interested,

Within the document there is a heated discussion about how the PR
template/review template should be.

Here share my opinion:

1. For the review template, actually we don't need comment a review
template at all. GitHub has a tag system and only committer could add tags,
which we can make use of it. That is, tagging this PR is
waiting-for-proposal-approved, waiting-for-code-review,
waiting-for-benchmark or block-by-author and so on. Asfbot could pick
GitHub tag state to the corresponding JIRA and we always regard JIRA as the
main discussion borad.

2. For the PR template, the greeting message is redundant. Just emphasize a
JIRA associated is important and how to format the title is enough.
Besides, the "Does this pull request potentially affect one of the
following parts" part and "Documentation" should be coved from "What is the
purpose of the change" and "Brief change log". These two parts, users
always answer no and would be aware if they really make changes on it. As
example, even pull request requires document, its owner might no add it at
first. The PR template is a guide but not which one have to learn.

To sum up, (1) take advantage of GitHub's tag system to tag review progress
(2) make the template more concise to avoid burden mature contributors and
force new comer to learn too much.

Best,
tison.


Rong Rong <[hidden email]> 于2018年9月18日周二 上午7:05写道:

> Thanks for putting the review contribution doc together, Stephan! This will
> definitely help the community to make the review process better.
>
> From my experience this will benefit on both contributors and reviewers
> side! Thus +1 for putting into practice as well.
>
> --
> Rong
>
> On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]> wrote:
>
> > Hi!
> >
> > Thanks you for the encouraging feedback so far.
> >
> > The overall goal is definitely to make the contribution process better
> and
> > get fewer pull requests that are disregarded.
> >
> > There are various reasons for the disregarded pull requests, one being
> that
> > fewer committers really participate in reviews beyond
> > the component they are currently very involved with. This is a separate
> > issue and I am thinking on how to encourage more
> > activity there.
> >
> > The other reason I was lack of structure and lack of decision making,
> which
> > is what I am first trying to fix here.
> > A follow-up to this will definitely be to improve the contribution guide
> as
> > well.
> >
> > Best,
> > Stephan
> >
> >
> > On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
> > [hidden email]> wrote:
> >
> > > From my personal experience as a contributor for three years, I feel
> > > better experience in contirbuting or reviewing than before, although we
> > > still have some points for further progress.
> > >
> > > I reviewed the proposal doc, and it gives very constructive and
> > meaningful
> > > guides which could help both contributor and reviewer. I agree with the
> > > bove suggestions and wish they can be praticed well!
> > >
> > > Best,
> > > Zhijiang
> > > ------------------------------------------------------------------
> > > 发件人:Till Rohrmann <[hidden email]>
> > > 发送时间:2018年9月17日(星期一) 16:27
> > > 收件人:dev <[hidden email]>
> > > 主 题:Re: [PROPOSAL] [community] A more structured approach to reviews
> and
> > > contributions
> > >
> > > Thanks for writing this up Stephan. I like the steps and hope that it
> > will
> > > help the community to make the review process better. Thus, +1 for
> > putting
> > > your proposal to practice.
> > >
> > > Cheers,
> > > Till
> > >
> > > On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]>
> wrote:
> > >
> > > > Hi Flink community members!
> > > >
> > > > As many of you will have noticed, the Flink project activity has gone
> > up
> > > > again quite a bit.
> > > > There are many more contributions, which is an absolutely great thing
> > to
> > > > have :-)
> > > >
> > > > However, we see a continuously growing backlog of pull requests and
> > JIRA
> > > > issues.
> > > > To make sure the community will be able to handle the increased
> > volume, I
> > > > think we need to revisit some
> > > > approaches and processes. I believe there are a few opportunities to
> > > > structure things a bit better, which
> > > > should help to scale the development.
> > > >
> > > > The first thing I would like to bring up are *Pull Request Reviews*.
> > Even
> > > > though more community members being
> > > > active in reviews (which is a really great thing!) the Pull Request
> > > backlog
> > > > is increasing quite a bit.
> > > >
> > > > Why are pull requests still not merged faster? Looking at the
> reviews,
> > > one
> > > > thing I noticed is that most reviews deal
> > > > immediately with detailed code issues, and leave out most of the core
> > > > questions that need to be answered
> > > > before a Pull Request can be merged, like "is this a desired
> feature?"
> > or
> > > > "does this align well with other developments?".
> > > > I think that we even make things slightly worse that way: From my
> > > personal
> > > > experience, I have often thought "oh, this
> > > > PR has a review already" and rather looked at another PR, only to
> find
> > > > later that the first review did never decide whether
> > > > this PR is actually a good fit for Flink.
> > > >
> > > > There has never been a proper documentation of how to answer these
> > > > questions, what to evaluate in reviews,
> > > > guidelines for how to evaluate pull requests, other than code
> quality.
> > I
> > > > suspect that this is why so many reviewers
> > > > do not address the "is this a good contribution" questions, making
> pull
> > > > requests linger until another committers joins
> > > > the review.
> > > >
> > > > Below is an idea for a guide *"How to Review Contributions"*. It
> > outlines
> > > > five core aspects to be checked in every
> > > > pull request, and suggests a priority for clarifying those. The idea
> is
> > > > that this helps us to better structure reviews, and
> > > > to make each reviewer aware what we look for in a review and where to
> > > best
> > > > bring in their help.
> > > >
> > > > Looking forward to comments!
> > > >
> > > > Best,
> > > > Stephan
> > > >
> > > > ====================================
> > > >
> > > > The draft is in this Google Doc. Please add small textual comments to
> > the
> > > > doc, and bigger principle discussions as replies to this mail.
> > > >
> > > >
> > > > https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
> > > RbocGlGKCYnvJd9lVhk/edit?usp=sharing
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > *How to Review Contributions------------------------------This guide
> is
> > > for
> > > > all committers and contributors that want to help with reviewing
> > > > contributions. Thank you for your effort - good reviews are one the
> > most
> > > > important and crucial parts of an open source project. This guide
> > should
> > > > help the community to make reviews such that: - Contributors have a
> > good
> > > > contribution experience- Reviews are structured and check all
> important
> > > > aspects of a contribution- Make sure we keep a high code quality in
> > > Flink-
> > > > We avoid situations where contributors and reviewers spend a lot of
> > time
> > > to
> > > > refine a contribution that gets rejected laterReview ChecklistEvery
> > > review
> > > > needs to check the following five aspects. We encourage to check
> these
> > > > aspects in order, to avoid spending time on detailed code quality
> > reviews
> > > > when there is not yet consensus that a feature or change should be
> > > actually
> > > > be added.(1) Is there consensus whether the change of feature should
> go
> > > > into to Flink?For bug fixes, this needs to be checked only in case it
> > > > requires bigger changes or might break existing programs and
> > > > setups.Ideally, this question is already answered from a JIRA issue
> or
> > > the
> > > > dev-list discussion, except in cases of bug fixes and small
> lightweight
> > > > additions/extensions. In that case, this question can be immediately
> > > marked
> > > > as resolved. For pull requests that are created without prior
> > consensus,
> > > > this question needs to be answered as part of the review.The decision
> > > > whether the change should go into Flink needs to take the following
> > > aspects
> > > > into consideration: - Does the contribution alter the behavior of
> > > features
> > > > or components in a way that it may break previous users’ programs and
> > > > setups? If yes, there needs to be a discussion and agreement that
> this
> > > > change is desirable. - Does the contribution conceptually fit well
> into
> > > > Flink? Is it too much of special case such that it makes things more
> > > > complicated for the common case, or bloats the abstractions / APIs? -
> > > Does
> > > > the feature fit well into Flink’s architecture? Will it scale and
> keep
> > > > Flink flexible for the future, or will the feature restrict Flink in
> > the
> > > > future? - Is the feature a significant new addition (rather than an
> > > > improvement to an existing part)? If yes, will the Flink community
> > commit
> > > > to maintaining this feature? - Does the feature produce added value
> for
> > > > Flink users or developers? Or does it introduce risk of regression
> > > without
> > > > adding relevant user or developer benefit?All of these questions
> should
> > > be
> > > > answerable from the description/discussion in JIRA and Pull Request,
> > > > without looking at the code.(2) Does the contribution need attention
> > from
> > > > some specific committers and is there time commitment from these
> > > > committers?Some changes require attention and approval from specific
> > > > committers. For example, changes in parts that are either very
> > > performance
> > > > sensitive, or have a critical impact on distributed coordination and
> > > fault
> > > > tolerance need input by a committer that is deeply familiar with the
> > > > component.As a rule of thumb, this is the case when the Pull Request
> > > > description answers one of the questions in the template section
> “Does
> > > this
> > > > pull request potentially affect one of the following parts” with
> > > ‘yes’.This
> > > > question can be answered with - Does not need specific attention-
> Needs
> > > > specific attention for X (X can be for example checkpointing,
> > jobmanager,
> > > > etc.).- Has specific attention for X by @commiterA, @contributorBIf
> the
> > > > pull request needs specific attention, one of the tagged
> > > > committers/contributors should give the final approval.(3) Is the
> > > > contribution described well?Check whether the contribution is
> > > sufficiently
> > > > well described to support a good review. Trivial changes and fixes do
> > not
> > > > need a long description. Any pull request that changes functionality
> or
> > > > behavior needs to describe the big picture of these changes, so that
> > > > reviews know what to look for (and don’t have to dig through the code
> > to
> > > > hopefully understand what the change does).Changes that require
> longer
> > > > descriptions are ideally based on a prior design discussion in the
> > > mailing
> > > > list or in JIRA and can simply link to there or copy the description
> > from
> > > > there.(4) Does the implementation follow the right overall
> > > > approach/architecture?Is this the best approach to implement the fix
> or
> > > > feature, or are there other approaches that would be easier, more
> > robust,
> > > > or more maintainable?This question should be answerable from the Pull
> > > > Request description (or the linked JIRA) as much as possible.We
> > recommend
> > > > to check this before diving into the details of commenting on
> > individual
> > > > parts of the change.(5) Is the overall code quality good, meeting
> > > standard
> > > > we want to maintain in Flink?This is the detailed code review of the
> > > actual
> > > > changes, covering: - Are the changes doing what is described in the
> > > design
> > > > document or PR description?- Does the code follow the right software
> > > > engineering practices? It the code correct, robust, maintainable,
> > > > testable?- Are the change performance aware, when changing a
> > performance
> > > > sensitive part?- Are the changes sufficiently covered by tests?- Are
> > the
> > > > tests executing fast?- Does the code format follow Flink’s checkstyle
> > > > pattern?- Does the code avoid to introduce additional compiler
> > > > warnings?Some code style guidelines can be found in the [Flink Code
> > Style
> > > > Page](https://flink.apache.org/contribute-code.html#code-style
> > > > <https://flink.apache.org/contribute-code.html#code-style>)Pull
> > Request
> > > > Review TemplateAdd the following checklist to the pull request
> review,
> > > > checking the boxes as the questions are answered:  - [ ] Consensus
> that
> > > the
> > > > contribution should go into to Flink  - [ ] Does not need specific
> > > > attention | Needs specific attention for X | Has attention for X by Y
> > -
> > > [
> > > > ] Contribution description  - [ ] Architectural approach  - [ ]
> Overall
> > > > code quality*
> > > >
> > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

Stephan Ewen
On the template discussion, some thoughts

*PR Template*

I think the PR template went well. We can rethink the "checklist" at the
bottom, but all other parts turned out helpful in my opinion.

With the amount of contributions, it helps to ask the contributor to take a
little more work in order for the reviewer to be more efficient.
I would suggest to keep that mindset: Whenever we find a way that the
contributor can prepare stuff in such a way that reviews become
more efficient, we should do that. In my experience, most contributors are
willing to put in some extra minutes if it helps that their
PR gets merged faster.

*Review Template*

I think it would be helpful to have this checklist. It does not matter in
which form, be that as a text template, be that as labels.

The most important thing is to make explicit which questions have been
answered in the review.
Currently there is a lot of "+1" on pull requests which means "code quality
is fine", but all other questions are unanswered.
The contributors then rightfully wonder why this does not get merged.



On Tue, Sep 18, 2018 at 7:26 AM, 陈梓立 <[hidden email]> wrote:

> Hi all interested,
>
> Within the document there is a heated discussion about how the PR
> template/review template should be.
>
> Here share my opinion:
>
> 1. For the review template, actually we don't need comment a review
> template at all. GitHub has a tag system and only committer could add tags,
> which we can make use of it. That is, tagging this PR is
> waiting-for-proposal-approved, waiting-for-code-review,
> waiting-for-benchmark or block-by-author and so on. Asfbot could pick
> GitHub tag state to the corresponding JIRA and we always regard JIRA as the
> main discussion borad.
>
> 2. For the PR template, the greeting message is redundant. Just emphasize a
> JIRA associated is important and how to format the title is enough.
> Besides, the "Does this pull request potentially affect one of the
> following parts" part and "Documentation" should be coved from "What is the
> purpose of the change" and "Brief change log". These two parts, users
> always answer no and would be aware if they really make changes on it. As
> example, even pull request requires document, its owner might no add it at
> first. The PR template is a guide but not which one have to learn.
>
> To sum up, (1) take advantage of GitHub's tag system to tag review progress
> (2) make the template more concise to avoid burden mature contributors and
> force new comer to learn too much.
>
> Best,
> tison.
>
>
> Rong Rong <[hidden email]> 于2018年9月18日周二 上午7:05写道:
>
> > Thanks for putting the review contribution doc together, Stephan! This
> will
> > definitely help the community to make the review process better.
> >
> > From my experience this will benefit on both contributors and reviewers
> > side! Thus +1 for putting into practice as well.
> >
> > --
> > Rong
> >
> > On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]> wrote:
> >
> > > Hi!
> > >
> > > Thanks you for the encouraging feedback so far.
> > >
> > > The overall goal is definitely to make the contribution process better
> > and
> > > get fewer pull requests that are disregarded.
> > >
> > > There are various reasons for the disregarded pull requests, one being
> > that
> > > fewer committers really participate in reviews beyond
> > > the component they are currently very involved with. This is a separate
> > > issue and I am thinking on how to encourage more
> > > activity there.
> > >
> > > The other reason I was lack of structure and lack of decision making,
> > which
> > > is what I am first trying to fix here.
> > > A follow-up to this will definitely be to improve the contribution
> guide
> > as
> > > well.
> > >
> > > Best,
> > > Stephan
> > >
> > >
> > > On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
> > > [hidden email]> wrote:
> > >
> > > > From my personal experience as a contributor for three years, I feel
> > > > better experience in contirbuting or reviewing than before, although
> we
> > > > still have some points for further progress.
> > > >
> > > > I reviewed the proposal doc, and it gives very constructive and
> > > meaningful
> > > > guides which could help both contributor and reviewer. I agree with
> the
> > > > bove suggestions and wish they can be praticed well!
> > > >
> > > > Best,
> > > > Zhijiang
> > > > ------------------------------------------------------------------
> > > > 发件人:Till Rohrmann <[hidden email]>
> > > > 发送时间:2018年9月17日(星期一) 16:27
> > > > 收件人:dev <[hidden email]>
> > > > 主 题:Re: [PROPOSAL] [community] A more structured approach to reviews
> > and
> > > > contributions
> > > >
> > > > Thanks for writing this up Stephan. I like the steps and hope that it
> > > will
> > > > help the community to make the review process better. Thus, +1 for
> > > putting
> > > > your proposal to practice.
> > > >
> > > > Cheers,
> > > > Till
> > > >
> > > > On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]>
> > wrote:
> > > >
> > > > > Hi Flink community members!
> > > > >
> > > > > As many of you will have noticed, the Flink project activity has
> gone
> > > up
> > > > > again quite a bit.
> > > > > There are many more contributions, which is an absolutely great
> thing
> > > to
> > > > > have :-)
> > > > >
> > > > > However, we see a continuously growing backlog of pull requests and
> > > JIRA
> > > > > issues.
> > > > > To make sure the community will be able to handle the increased
> > > volume, I
> > > > > think we need to revisit some
> > > > > approaches and processes. I believe there are a few opportunities
> to
> > > > > structure things a bit better, which
> > > > > should help to scale the development.
> > > > >
> > > > > The first thing I would like to bring up are *Pull Request
> Reviews*.
> > > Even
> > > > > though more community members being
> > > > > active in reviews (which is a really great thing!) the Pull Request
> > > > backlog
> > > > > is increasing quite a bit.
> > > > >
> > > > > Why are pull requests still not merged faster? Looking at the
> > reviews,
> > > > one
> > > > > thing I noticed is that most reviews deal
> > > > > immediately with detailed code issues, and leave out most of the
> core
> > > > > questions that need to be answered
> > > > > before a Pull Request can be merged, like "is this a desired
> > feature?"
> > > or
> > > > > "does this align well with other developments?".
> > > > > I think that we even make things slightly worse that way: From my
> > > > personal
> > > > > experience, I have often thought "oh, this
> > > > > PR has a review already" and rather looked at another PR, only to
> > find
> > > > > later that the first review did never decide whether
> > > > > this PR is actually a good fit for Flink.
> > > > >
> > > > > There has never been a proper documentation of how to answer these
> > > > > questions, what to evaluate in reviews,
> > > > > guidelines for how to evaluate pull requests, other than code
> > quality.
> > > I
> > > > > suspect that this is why so many reviewers
> > > > > do not address the "is this a good contribution" questions, making
> > pull
> > > > > requests linger until another committers joins
> > > > > the review.
> > > > >
> > > > > Below is an idea for a guide *"How to Review Contributions"*. It
> > > outlines
> > > > > five core aspects to be checked in every
> > > > > pull request, and suggests a priority for clarifying those. The
> idea
> > is
> > > > > that this helps us to better structure reviews, and
> > > > > to make each reviewer aware what we look for in a review and where
> to
> > > > best
> > > > > bring in their help.
> > > > >
> > > > > Looking forward to comments!
> > > > >
> > > > > Best,
> > > > > Stephan
> > > > >
> > > > > ====================================
> > > > >
> > > > > The draft is in this Google Doc. Please add small textual comments
> to
> > > the
> > > > > doc, and bigger principle discussions as replies to this mail.
> > > > >
> > > > >
> > > > > https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
> > > > RbocGlGKCYnvJd9lVhk/edit?usp=sharing
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > *How to Review Contributions------------------------------This
> guide
> > is
> > > > for
> > > > > all committers and contributors that want to help with reviewing
> > > > > contributions. Thank you for your effort - good reviews are one the
> > > most
> > > > > important and crucial parts of an open source project. This guide
> > > should
> > > > > help the community to make reviews such that: - Contributors have a
> > > good
> > > > > contribution experience- Reviews are structured and check all
> > important
> > > > > aspects of a contribution- Make sure we keep a high code quality in
> > > > Flink-
> > > > > We avoid situations where contributors and reviewers spend a lot of
> > > time
> > > > to
> > > > > refine a contribution that gets rejected laterReview ChecklistEvery
> > > > review
> > > > > needs to check the following five aspects. We encourage to check
> > these
> > > > > aspects in order, to avoid spending time on detailed code quality
> > > reviews
> > > > > when there is not yet consensus that a feature or change should be
> > > > actually
> > > > > be added.(1) Is there consensus whether the change of feature
> should
> > go
> > > > > into to Flink?For bug fixes, this needs to be checked only in case
> it
> > > > > requires bigger changes or might break existing programs and
> > > > > setups.Ideally, this question is already answered from a JIRA issue
> > or
> > > > the
> > > > > dev-list discussion, except in cases of bug fixes and small
> > lightweight
> > > > > additions/extensions. In that case, this question can be
> immediately
> > > > marked
> > > > > as resolved. For pull requests that are created without prior
> > > consensus,
> > > > > this question needs to be answered as part of the review.The
> decision
> > > > > whether the change should go into Flink needs to take the following
> > > > aspects
> > > > > into consideration: - Does the contribution alter the behavior of
> > > > features
> > > > > or components in a way that it may break previous users’ programs
> and
> > > > > setups? If yes, there needs to be a discussion and agreement that
> > this
> > > > > change is desirable. - Does the contribution conceptually fit well
> > into
> > > > > Flink? Is it too much of special case such that it makes things
> more
> > > > > complicated for the common case, or bloats the abstractions /
> APIs? -
> > > > Does
> > > > > the feature fit well into Flink’s architecture? Will it scale and
> > keep
> > > > > Flink flexible for the future, or will the feature restrict Flink
> in
> > > the
> > > > > future? - Is the feature a significant new addition (rather than an
> > > > > improvement to an existing part)? If yes, will the Flink community
> > > commit
> > > > > to maintaining this feature? - Does the feature produce added value
> > for
> > > > > Flink users or developers? Or does it introduce risk of regression
> > > > without
> > > > > adding relevant user or developer benefit?All of these questions
> > should
> > > > be
> > > > > answerable from the description/discussion in JIRA and Pull
> Request,
> > > > > without looking at the code.(2) Does the contribution need
> attention
> > > from
> > > > > some specific committers and is there time commitment from these
> > > > > committers?Some changes require attention and approval from
> specific
> > > > > committers. For example, changes in parts that are either very
> > > > performance
> > > > > sensitive, or have a critical impact on distributed coordination
> and
> > > > fault
> > > > > tolerance need input by a committer that is deeply familiar with
> the
> > > > > component.As a rule of thumb, this is the case when the Pull
> Request
> > > > > description answers one of the questions in the template section
> > “Does
> > > > this
> > > > > pull request potentially affect one of the following parts” with
> > > > ‘yes’.This
> > > > > question can be answered with - Does not need specific attention-
> > Needs
> > > > > specific attention for X (X can be for example checkpointing,
> > > jobmanager,
> > > > > etc.).- Has specific attention for X by @commiterA, @contributorBIf
> > the
> > > > > pull request needs specific attention, one of the tagged
> > > > > committers/contributors should give the final approval.(3) Is the
> > > > > contribution described well?Check whether the contribution is
> > > > sufficiently
> > > > > well described to support a good review. Trivial changes and fixes
> do
> > > not
> > > > > need a long description. Any pull request that changes
> functionality
> > or
> > > > > behavior needs to describe the big picture of these changes, so
> that
> > > > > reviews know what to look for (and don’t have to dig through the
> code
> > > to
> > > > > hopefully understand what the change does).Changes that require
> > longer
> > > > > descriptions are ideally based on a prior design discussion in the
> > > > mailing
> > > > > list or in JIRA and can simply link to there or copy the
> description
> > > from
> > > > > there.(4) Does the implementation follow the right overall
> > > > > approach/architecture?Is this the best approach to implement the
> fix
> > or
> > > > > feature, or are there other approaches that would be easier, more
> > > robust,
> > > > > or more maintainable?This question should be answerable from the
> Pull
> > > > > Request description (or the linked JIRA) as much as possible.We
> > > recommend
> > > > > to check this before diving into the details of commenting on
> > > individual
> > > > > parts of the change.(5) Is the overall code quality good, meeting
> > > > standard
> > > > > we want to maintain in Flink?This is the detailed code review of
> the
> > > > actual
> > > > > changes, covering: - Are the changes doing what is described in the
> > > > design
> > > > > document or PR description?- Does the code follow the right
> software
> > > > > engineering practices? It the code correct, robust, maintainable,
> > > > > testable?- Are the change performance aware, when changing a
> > > performance
> > > > > sensitive part?- Are the changes sufficiently covered by tests?-
> Are
> > > the
> > > > > tests executing fast?- Does the code format follow Flink’s
> checkstyle
> > > > > pattern?- Does the code avoid to introduce additional compiler
> > > > > warnings?Some code style guidelines can be found in the [Flink Code
> > > Style
> > > > > Page](https://flink.apache.org/contribute-code.html#code-style
> > > > > <https://flink.apache.org/contribute-code.html#code-style>)Pull
> > > Request
> > > > > Review TemplateAdd the following checklist to the pull request
> > review,
> > > > > checking the boxes as the questions are answered:  - [ ] Consensus
> > that
> > > > the
> > > > > contribution should go into to Flink  - [ ] Does not need specific
> > > > > attention | Needs specific attention for X | Has attention for X
> by Y
> > > -
> > > > [
> > > > > ] Contribution description  - [ ] Architectural approach  - [ ]
> > Overall
> > > > > code quality*
> > > > >
> > > >
> > > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

tison
Maybe a little rearrange to the process would help.

(1). Does the contributor describe itself well?
  (1.1) By whom this contribution should be given attention. This often
shows by its title, "[FLINK-XXX] [module]", the module part infer.
  (1.2) What the purpose of this contribution is. Done by the PR template.
Even on JIRA an issue should cover these points.

(2). Is there consensus on the contribution?
This follows (1), because we need to clear what the purpose of the
contribution first. At this stage reviewers could cc to module maintainer
as a supplement to (1.1). Also reviewers might ask the contributor to
clarify his purpose to sharp(1.2)

(3). Is the implement architectural and fit code style?
This follows (2). And only after a consensus we talk about concrete
implement, which prevent spend time and put effort in vain.

In addition, ideally a "+1" comment or approval means the purpose of
contribution is supported by the reviewer and implement(if there is)
quality is fine, so the reviewer vote for a consensus.

Best,
tison.


Stephan Ewen <[hidden email]> 于2018年9月18日周二 下午6:44写道:

> On the template discussion, some thoughts
>
> *PR Template*
>
> I think the PR template went well. We can rethink the "checklist" at the
> bottom, but all other parts turned out helpful in my opinion.
>
> With the amount of contributions, it helps to ask the contributor to take a
> little more work in order for the reviewer to be more efficient.
> I would suggest to keep that mindset: Whenever we find a way that the
> contributor can prepare stuff in such a way that reviews become
> more efficient, we should do that. In my experience, most contributors are
> willing to put in some extra minutes if it helps that their
> PR gets merged faster.
>
> *Review Template*
>
> I think it would be helpful to have this checklist. It does not matter in
> which form, be that as a text template, be that as labels.
>
> The most important thing is to make explicit which questions have been
> answered in the review.
> Currently there is a lot of "+1" on pull requests which means "code quality
> is fine", but all other questions are unanswered.
> The contributors then rightfully wonder why this does not get merged.
>
>
>
> On Tue, Sep 18, 2018 at 7:26 AM, 陈梓立 <[hidden email]> wrote:
>
> > Hi all interested,
> >
> > Within the document there is a heated discussion about how the PR
> > template/review template should be.
> >
> > Here share my opinion:
> >
> > 1. For the review template, actually we don't need comment a review
> > template at all. GitHub has a tag system and only committer could add
> tags,
> > which we can make use of it. That is, tagging this PR is
> > waiting-for-proposal-approved, waiting-for-code-review,
> > waiting-for-benchmark or block-by-author and so on. Asfbot could pick
> > GitHub tag state to the corresponding JIRA and we always regard JIRA as
> the
> > main discussion borad.
> >
> > 2. For the PR template, the greeting message is redundant. Just
> emphasize a
> > JIRA associated is important and how to format the title is enough.
> > Besides, the "Does this pull request potentially affect one of the
> > following parts" part and "Documentation" should be coved from "What is
> the
> > purpose of the change" and "Brief change log". These two parts, users
> > always answer no and would be aware if they really make changes on it. As
> > example, even pull request requires document, its owner might no add it
> at
> > first. The PR template is a guide but not which one have to learn.
> >
> > To sum up, (1) take advantage of GitHub's tag system to tag review
> progress
> > (2) make the template more concise to avoid burden mature contributors
> and
> > force new comer to learn too much.
> >
> > Best,
> > tison.
> >
> >
> > Rong Rong <[hidden email]> 于2018年9月18日周二 上午7:05写道:
> >
> > > Thanks for putting the review contribution doc together, Stephan! This
> > will
> > > definitely help the community to make the review process better.
> > >
> > > From my experience this will benefit on both contributors and reviewers
> > > side! Thus +1 for putting into practice as well.
> > >
> > > --
> > > Rong
> > >
> > > On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]>
> wrote:
> > >
> > > > Hi!
> > > >
> > > > Thanks you for the encouraging feedback so far.
> > > >
> > > > The overall goal is definitely to make the contribution process
> better
> > > and
> > > > get fewer pull requests that are disregarded.
> > > >
> > > > There are various reasons for the disregarded pull requests, one
> being
> > > that
> > > > fewer committers really participate in reviews beyond
> > > > the component they are currently very involved with. This is a
> separate
> > > > issue and I am thinking on how to encourage more
> > > > activity there.
> > > >
> > > > The other reason I was lack of structure and lack of decision making,
> > > which
> > > > is what I am first trying to fix here.
> > > > A follow-up to this will definitely be to improve the contribution
> > guide
> > > as
> > > > well.
> > > >
> > > > Best,
> > > > Stephan
> > > >
> > > >
> > > > On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
> > > > [hidden email]> wrote:
> > > >
> > > > > From my personal experience as a contributor for three years, I
> feel
> > > > > better experience in contirbuting or reviewing than before,
> although
> > we
> > > > > still have some points for further progress.
> > > > >
> > > > > I reviewed the proposal doc, and it gives very constructive and
> > > > meaningful
> > > > > guides which could help both contributor and reviewer. I agree with
> > the
> > > > > bove suggestions and wish they can be praticed well!
> > > > >
> > > > > Best,
> > > > > Zhijiang
> > > > > ------------------------------------------------------------------
> > > > > 发件人:Till Rohrmann <[hidden email]>
> > > > > 发送时间:2018年9月17日(星期一) 16:27
> > > > > 收件人:dev <[hidden email]>
> > > > > 主 题:Re: [PROPOSAL] [community] A more structured approach to
> reviews
> > > and
> > > > > contributions
> > > > >
> > > > > Thanks for writing this up Stephan. I like the steps and hope that
> it
> > > > will
> > > > > help the community to make the review process better. Thus, +1 for
> > > > putting
> > > > > your proposal to practice.
> > > > >
> > > > > Cheers,
> > > > > Till
> > > > >
> > > > > On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]>
> > > wrote:
> > > > >
> > > > > > Hi Flink community members!
> > > > > >
> > > > > > As many of you will have noticed, the Flink project activity has
> > gone
> > > > up
> > > > > > again quite a bit.
> > > > > > There are many more contributions, which is an absolutely great
> > thing
> > > > to
> > > > > > have :-)
> > > > > >
> > > > > > However, we see a continuously growing backlog of pull requests
> and
> > > > JIRA
> > > > > > issues.
> > > > > > To make sure the community will be able to handle the increased
> > > > volume, I
> > > > > > think we need to revisit some
> > > > > > approaches and processes. I believe there are a few opportunities
> > to
> > > > > > structure things a bit better, which
> > > > > > should help to scale the development.
> > > > > >
> > > > > > The first thing I would like to bring up are *Pull Request
> > Reviews*.
> > > > Even
> > > > > > though more community members being
> > > > > > active in reviews (which is a really great thing!) the Pull
> Request
> > > > > backlog
> > > > > > is increasing quite a bit.
> > > > > >
> > > > > > Why are pull requests still not merged faster? Looking at the
> > > reviews,
> > > > > one
> > > > > > thing I noticed is that most reviews deal
> > > > > > immediately with detailed code issues, and leave out most of the
> > core
> > > > > > questions that need to be answered
> > > > > > before a Pull Request can be merged, like "is this a desired
> > > feature?"
> > > > or
> > > > > > "does this align well with other developments?".
> > > > > > I think that we even make things slightly worse that way: From my
> > > > > personal
> > > > > > experience, I have often thought "oh, this
> > > > > > PR has a review already" and rather looked at another PR, only to
> > > find
> > > > > > later that the first review did never decide whether
> > > > > > this PR is actually a good fit for Flink.
> > > > > >
> > > > > > There has never been a proper documentation of how to answer
> these
> > > > > > questions, what to evaluate in reviews,
> > > > > > guidelines for how to evaluate pull requests, other than code
> > > quality.
> > > > I
> > > > > > suspect that this is why so many reviewers
> > > > > > do not address the "is this a good contribution" questions,
> making
> > > pull
> > > > > > requests linger until another committers joins
> > > > > > the review.
> > > > > >
> > > > > > Below is an idea for a guide *"How to Review Contributions"*. It
> > > > outlines
> > > > > > five core aspects to be checked in every
> > > > > > pull request, and suggests a priority for clarifying those. The
> > idea
> > > is
> > > > > > that this helps us to better structure reviews, and
> > > > > > to make each reviewer aware what we look for in a review and
> where
> > to
> > > > > best
> > > > > > bring in their help.
> > > > > >
> > > > > > Looking forward to comments!
> > > > > >
> > > > > > Best,
> > > > > > Stephan
> > > > > >
> > > > > > ====================================
> > > > > >
> > > > > > The draft is in this Google Doc. Please add small textual
> comments
> > to
> > > > the
> > > > > > doc, and bigger principle discussions as replies to this mail.
> > > > > >
> > > > > >
> > > > > > https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
> > > > > RbocGlGKCYnvJd9lVhk/edit?usp=sharing
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > *How to Review Contributions------------------------------This
> > guide
> > > is
> > > > > for
> > > > > > all committers and contributors that want to help with reviewing
> > > > > > contributions. Thank you for your effort - good reviews are one
> the
> > > > most
> > > > > > important and crucial parts of an open source project. This guide
> > > > should
> > > > > > help the community to make reviews such that: - Contributors
> have a
> > > > good
> > > > > > contribution experience- Reviews are structured and check all
> > > important
> > > > > > aspects of a contribution- Make sure we keep a high code quality
> in
> > > > > Flink-
> > > > > > We avoid situations where contributors and reviewers spend a lot
> of
> > > > time
> > > > > to
> > > > > > refine a contribution that gets rejected laterReview
> ChecklistEvery
> > > > > review
> > > > > > needs to check the following five aspects. We encourage to check
> > > these
> > > > > > aspects in order, to avoid spending time on detailed code quality
> > > > reviews
> > > > > > when there is not yet consensus that a feature or change should
> be
> > > > > actually
> > > > > > be added.(1) Is there consensus whether the change of feature
> > should
> > > go
> > > > > > into to Flink?For bug fixes, this needs to be checked only in
> case
> > it
> > > > > > requires bigger changes or might break existing programs and
> > > > > > setups.Ideally, this question is already answered from a JIRA
> issue
> > > or
> > > > > the
> > > > > > dev-list discussion, except in cases of bug fixes and small
> > > lightweight
> > > > > > additions/extensions. In that case, this question can be
> > immediately
> > > > > marked
> > > > > > as resolved. For pull requests that are created without prior
> > > > consensus,
> > > > > > this question needs to be answered as part of the review.The
> > decision
> > > > > > whether the change should go into Flink needs to take the
> following
> > > > > aspects
> > > > > > into consideration: - Does the contribution alter the behavior of
> > > > > features
> > > > > > or components in a way that it may break previous users’ programs
> > and
> > > > > > setups? If yes, there needs to be a discussion and agreement that
> > > this
> > > > > > change is desirable. - Does the contribution conceptually fit
> well
> > > into
> > > > > > Flink? Is it too much of special case such that it makes things
> > more
> > > > > > complicated for the common case, or bloats the abstractions /
> > APIs? -
> > > > > Does
> > > > > > the feature fit well into Flink’s architecture? Will it scale and
> > > keep
> > > > > > Flink flexible for the future, or will the feature restrict Flink
> > in
> > > > the
> > > > > > future? - Is the feature a significant new addition (rather than
> an
> > > > > > improvement to an existing part)? If yes, will the Flink
> community
> > > > commit
> > > > > > to maintaining this feature? - Does the feature produce added
> value
> > > for
> > > > > > Flink users or developers? Or does it introduce risk of
> regression
> > > > > without
> > > > > > adding relevant user or developer benefit?All of these questions
> > > should
> > > > > be
> > > > > > answerable from the description/discussion in JIRA and Pull
> > Request,
> > > > > > without looking at the code.(2) Does the contribution need
> > attention
> > > > from
> > > > > > some specific committers and is there time commitment from these
> > > > > > committers?Some changes require attention and approval from
> > specific
> > > > > > committers. For example, changes in parts that are either very
> > > > > performance
> > > > > > sensitive, or have a critical impact on distributed coordination
> > and
> > > > > fault
> > > > > > tolerance need input by a committer that is deeply familiar with
> > the
> > > > > > component.As a rule of thumb, this is the case when the Pull
> > Request
> > > > > > description answers one of the questions in the template section
> > > “Does
> > > > > this
> > > > > > pull request potentially affect one of the following parts” with
> > > > > ‘yes’.This
> > > > > > question can be answered with - Does not need specific attention-
> > > Needs
> > > > > > specific attention for X (X can be for example checkpointing,
> > > > jobmanager,
> > > > > > etc.).- Has specific attention for X by @commiterA,
> @contributorBIf
> > > the
> > > > > > pull request needs specific attention, one of the tagged
> > > > > > committers/contributors should give the final approval.(3) Is the
> > > > > > contribution described well?Check whether the contribution is
> > > > > sufficiently
> > > > > > well described to support a good review. Trivial changes and
> fixes
> > do
> > > > not
> > > > > > need a long description. Any pull request that changes
> > functionality
> > > or
> > > > > > behavior needs to describe the big picture of these changes, so
> > that
> > > > > > reviews know what to look for (and don’t have to dig through the
> > code
> > > > to
> > > > > > hopefully understand what the change does).Changes that require
> > > longer
> > > > > > descriptions are ideally based on a prior design discussion in
> the
> > > > > mailing
> > > > > > list or in JIRA and can simply link to there or copy the
> > description
> > > > from
> > > > > > there.(4) Does the implementation follow the right overall
> > > > > > approach/architecture?Is this the best approach to implement the
> > fix
> > > or
> > > > > > feature, or are there other approaches that would be easier, more
> > > > robust,
> > > > > > or more maintainable?This question should be answerable from the
> > Pull
> > > > > > Request description (or the linked JIRA) as much as possible.We
> > > > recommend
> > > > > > to check this before diving into the details of commenting on
> > > > individual
> > > > > > parts of the change.(5) Is the overall code quality good, meeting
> > > > > standard
> > > > > > we want to maintain in Flink?This is the detailed code review of
> > the
> > > > > actual
> > > > > > changes, covering: - Are the changes doing what is described in
> the
> > > > > design
> > > > > > document or PR description?- Does the code follow the right
> > software
> > > > > > engineering practices? It the code correct, robust, maintainable,
> > > > > > testable?- Are the change performance aware, when changing a
> > > > performance
> > > > > > sensitive part?- Are the changes sufficiently covered by tests?-
> > Are
> > > > the
> > > > > > tests executing fast?- Does the code format follow Flink’s
> > checkstyle
> > > > > > pattern?- Does the code avoid to introduce additional compiler
> > > > > > warnings?Some code style guidelines can be found in the [Flink
> Code
> > > > Style
> > > > > > Page](https://flink.apache.org/contribute-code.html#code-style
> > > > > > <https://flink.apache.org/contribute-code.html#code-style>)Pull
> > > > Request
> > > > > > Review TemplateAdd the following checklist to the pull request
> > > review,
> > > > > > checking the boxes as the questions are answered:  - [ ]
> Consensus
> > > that
> > > > > the
> > > > > > contribution should go into to Flink  - [ ] Does not need
> specific
> > > > > > attention | Needs specific attention for X | Has attention for X
> > by Y
> > > > -
> > > > > [
> > > > > > ] Contribution description  - [ ] Architectural approach  - [ ]
> > > Overall
> > > > > > code quality*
> > > > > >
> > > > >
> > > > >
> > > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

tison
Put some good cases here might be helpful.

See how a contribution of runtime module be proposed, discussed,
implemented and merged from  https://github.com/apache/flink/pull/5931 to
https://github.com/apache/flink/pull/6132.

1. #5931 fix a bug, but remains points could be improved. Here sihuazhou
and shuai-xu share their considerations and require review(of the proposal)
by Stephan, Till and Gary, our committers.
2. After discussion, all people involved reach a consensus. So the rest
work is to implement it.
3. sihuazhou gives out an implementation #6132, Till reviews it and find it
is somewhat out of the "architectural" aspect, so suggests
implementation-level changes.
4. Addressing those implementation-level comments, the PR gets merged.

I think this is quite a good example how we think our review process should
go.

Best,
tison.


陈梓立 <[hidden email]> 于2018年9月18日周二 下午10:53写道:

> Maybe a little rearrange to the process would help.
>
> (1). Does the contributor describe itself well?
>   (1.1) By whom this contribution should be given attention. This often
> shows by its title, "[FLINK-XXX] [module]", the module part infer.
>   (1.2) What the purpose of this contribution is. Done by the PR template.
> Even on JIRA an issue should cover these points.
>
> (2). Is there consensus on the contribution?
> This follows (1), because we need to clear what the purpose of the
> contribution first. At this stage reviewers could cc to module maintainer
> as a supplement to (1.1). Also reviewers might ask the contributor to
> clarify his purpose to sharp(1.2)
>
> (3). Is the implement architectural and fit code style?
> This follows (2). And only after a consensus we talk about concrete
> implement, which prevent spend time and put effort in vain.
>
> In addition, ideally a "+1" comment or approval means the purpose of
> contribution is supported by the reviewer and implement(if there is)
> quality is fine, so the reviewer vote for a consensus.
>
> Best,
> tison.
>
>
> Stephan Ewen <[hidden email]> 于2018年9月18日周二 下午6:44写道:
>
>> On the template discussion, some thoughts
>>
>> *PR Template*
>>
>> I think the PR template went well. We can rethink the "checklist" at the
>> bottom, but all other parts turned out helpful in my opinion.
>>
>> With the amount of contributions, it helps to ask the contributor to take
>> a
>> little more work in order for the reviewer to be more efficient.
>> I would suggest to keep that mindset: Whenever we find a way that the
>> contributor can prepare stuff in such a way that reviews become
>> more efficient, we should do that. In my experience, most contributors are
>> willing to put in some extra minutes if it helps that their
>> PR gets merged faster.
>>
>> *Review Template*
>>
>> I think it would be helpful to have this checklist. It does not matter in
>> which form, be that as a text template, be that as labels.
>>
>> The most important thing is to make explicit which questions have been
>> answered in the review.
>> Currently there is a lot of "+1" on pull requests which means "code
>> quality
>> is fine", but all other questions are unanswered.
>> The contributors then rightfully wonder why this does not get merged.
>>
>>
>>
>> On Tue, Sep 18, 2018 at 7:26 AM, 陈梓立 <[hidden email]> wrote:
>>
>> > Hi all interested,
>> >
>> > Within the document there is a heated discussion about how the PR
>> > template/review template should be.
>> >
>> > Here share my opinion:
>> >
>> > 1. For the review template, actually we don't need comment a review
>> > template at all. GitHub has a tag system and only committer could add
>> tags,
>> > which we can make use of it. That is, tagging this PR is
>> > waiting-for-proposal-approved, waiting-for-code-review,
>> > waiting-for-benchmark or block-by-author and so on. Asfbot could pick
>> > GitHub tag state to the corresponding JIRA and we always regard JIRA as
>> the
>> > main discussion borad.
>> >
>> > 2. For the PR template, the greeting message is redundant. Just
>> emphasize a
>> > JIRA associated is important and how to format the title is enough.
>> > Besides, the "Does this pull request potentially affect one of the
>> > following parts" part and "Documentation" should be coved from "What is
>> the
>> > purpose of the change" and "Brief change log". These two parts, users
>> > always answer no and would be aware if they really make changes on it.
>> As
>> > example, even pull request requires document, its owner might no add it
>> at
>> > first. The PR template is a guide but not which one have to learn.
>> >
>> > To sum up, (1) take advantage of GitHub's tag system to tag review
>> progress
>> > (2) make the template more concise to avoid burden mature contributors
>> and
>> > force new comer to learn too much.
>> >
>> > Best,
>> > tison.
>> >
>> >
>> > Rong Rong <[hidden email]> 于2018年9月18日周二 上午7:05写道:
>> >
>> > > Thanks for putting the review contribution doc together, Stephan! This
>> > will
>> > > definitely help the community to make the review process better.
>> > >
>> > > From my experience this will benefit on both contributors and
>> reviewers
>> > > side! Thus +1 for putting into practice as well.
>> > >
>> > > --
>> > > Rong
>> > >
>> > > On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]>
>> wrote:
>> > >
>> > > > Hi!
>> > > >
>> > > > Thanks you for the encouraging feedback so far.
>> > > >
>> > > > The overall goal is definitely to make the contribution process
>> better
>> > > and
>> > > > get fewer pull requests that are disregarded.
>> > > >
>> > > > There are various reasons for the disregarded pull requests, one
>> being
>> > > that
>> > > > fewer committers really participate in reviews beyond
>> > > > the component they are currently very involved with. This is a
>> separate
>> > > > issue and I am thinking on how to encourage more
>> > > > activity there.
>> > > >
>> > > > The other reason I was lack of structure and lack of decision
>> making,
>> > > which
>> > > > is what I am first trying to fix here.
>> > > > A follow-up to this will definitely be to improve the contribution
>> > guide
>> > > as
>> > > > well.
>> > > >
>> > > > Best,
>> > > > Stephan
>> > > >
>> > > >
>> > > > On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
>> > > > [hidden email]> wrote:
>> > > >
>> > > > > From my personal experience as a contributor for three years, I
>> feel
>> > > > > better experience in contirbuting or reviewing than before,
>> although
>> > we
>> > > > > still have some points for further progress.
>> > > > >
>> > > > > I reviewed the proposal doc, and it gives very constructive and
>> > > > meaningful
>> > > > > guides which could help both contributor and reviewer. I agree
>> with
>> > the
>> > > > > bove suggestions and wish they can be praticed well!
>> > > > >
>> > > > > Best,
>> > > > > Zhijiang
>> > > > > ------------------------------------------------------------------
>> > > > > 发件人:Till Rohrmann <[hidden email]>
>> > > > > 发送时间:2018年9月17日(星期一) 16:27
>> > > > > 收件人:dev <[hidden email]>
>> > > > > 主 题:Re: [PROPOSAL] [community] A more structured approach to
>> reviews
>> > > and
>> > > > > contributions
>> > > > >
>> > > > > Thanks for writing this up Stephan. I like the steps and hope
>> that it
>> > > > will
>> > > > > help the community to make the review process better. Thus, +1 for
>> > > > putting
>> > > > > your proposal to practice.
>> > > > >
>> > > > > Cheers,
>> > > > > Till
>> > > > >
>> > > > > On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]>
>> > > wrote:
>> > > > >
>> > > > > > Hi Flink community members!
>> > > > > >
>> > > > > > As many of you will have noticed, the Flink project activity has
>> > gone
>> > > > up
>> > > > > > again quite a bit.
>> > > > > > There are many more contributions, which is an absolutely great
>> > thing
>> > > > to
>> > > > > > have :-)
>> > > > > >
>> > > > > > However, we see a continuously growing backlog of pull requests
>> and
>> > > > JIRA
>> > > > > > issues.
>> > > > > > To make sure the community will be able to handle the increased
>> > > > volume, I
>> > > > > > think we need to revisit some
>> > > > > > approaches and processes. I believe there are a few
>> opportunities
>> > to
>> > > > > > structure things a bit better, which
>> > > > > > should help to scale the development.
>> > > > > >
>> > > > > > The first thing I would like to bring up are *Pull Request
>> > Reviews*.
>> > > > Even
>> > > > > > though more community members being
>> > > > > > active in reviews (which is a really great thing!) the Pull
>> Request
>> > > > > backlog
>> > > > > > is increasing quite a bit.
>> > > > > >
>> > > > > > Why are pull requests still not merged faster? Looking at the
>> > > reviews,
>> > > > > one
>> > > > > > thing I noticed is that most reviews deal
>> > > > > > immediately with detailed code issues, and leave out most of the
>> > core
>> > > > > > questions that need to be answered
>> > > > > > before a Pull Request can be merged, like "is this a desired
>> > > feature?"
>> > > > or
>> > > > > > "does this align well with other developments?".
>> > > > > > I think that we even make things slightly worse that way: From
>> my
>> > > > > personal
>> > > > > > experience, I have often thought "oh, this
>> > > > > > PR has a review already" and rather looked at another PR, only
>> to
>> > > find
>> > > > > > later that the first review did never decide whether
>> > > > > > this PR is actually a good fit for Flink.
>> > > > > >
>> > > > > > There has never been a proper documentation of how to answer
>> these
>> > > > > > questions, what to evaluate in reviews,
>> > > > > > guidelines for how to evaluate pull requests, other than code
>> > > quality.
>> > > > I
>> > > > > > suspect that this is why so many reviewers
>> > > > > > do not address the "is this a good contribution" questions,
>> making
>> > > pull
>> > > > > > requests linger until another committers joins
>> > > > > > the review.
>> > > > > >
>> > > > > > Below is an idea for a guide *"How to Review Contributions"*. It
>> > > > outlines
>> > > > > > five core aspects to be checked in every
>> > > > > > pull request, and suggests a priority for clarifying those. The
>> > idea
>> > > is
>> > > > > > that this helps us to better structure reviews, and
>> > > > > > to make each reviewer aware what we look for in a review and
>> where
>> > to
>> > > > > best
>> > > > > > bring in their help.
>> > > > > >
>> > > > > > Looking forward to comments!
>> > > > > >
>> > > > > > Best,
>> > > > > > Stephan
>> > > > > >
>> > > > > > ====================================
>> > > > > >
>> > > > > > The draft is in this Google Doc. Please add small textual
>> comments
>> > to
>> > > > the
>> > > > > > doc, and bigger principle discussions as replies to this mail.
>> > > > > >
>> > > > > >
>> > > > > > https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
>> > > > > RbocGlGKCYnvJd9lVhk/edit?usp=sharing
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > *How to Review Contributions------------------------------This
>> > guide
>> > > is
>> > > > > for
>> > > > > > all committers and contributors that want to help with reviewing
>> > > > > > contributions. Thank you for your effort - good reviews are one
>> the
>> > > > most
>> > > > > > important and crucial parts of an open source project. This
>> guide
>> > > > should
>> > > > > > help the community to make reviews such that: - Contributors
>> have a
>> > > > good
>> > > > > > contribution experience- Reviews are structured and check all
>> > > important
>> > > > > > aspects of a contribution- Make sure we keep a high code
>> quality in
>> > > > > Flink-
>> > > > > > We avoid situations where contributors and reviewers spend a
>> lot of
>> > > > time
>> > > > > to
>> > > > > > refine a contribution that gets rejected laterReview
>> ChecklistEvery
>> > > > > review
>> > > > > > needs to check the following five aspects. We encourage to check
>> > > these
>> > > > > > aspects in order, to avoid spending time on detailed code
>> quality
>> > > > reviews
>> > > > > > when there is not yet consensus that a feature or change should
>> be
>> > > > > actually
>> > > > > > be added.(1) Is there consensus whether the change of feature
>> > should
>> > > go
>> > > > > > into to Flink?For bug fixes, this needs to be checked only in
>> case
>> > it
>> > > > > > requires bigger changes or might break existing programs and
>> > > > > > setups.Ideally, this question is already answered from a JIRA
>> issue
>> > > or
>> > > > > the
>> > > > > > dev-list discussion, except in cases of bug fixes and small
>> > > lightweight
>> > > > > > additions/extensions. In that case, this question can be
>> > immediately
>> > > > > marked
>> > > > > > as resolved. For pull requests that are created without prior
>> > > > consensus,
>> > > > > > this question needs to be answered as part of the review.The
>> > decision
>> > > > > > whether the change should go into Flink needs to take the
>> following
>> > > > > aspects
>> > > > > > into consideration: - Does the contribution alter the behavior
>> of
>> > > > > features
>> > > > > > or components in a way that it may break previous users’
>> programs
>> > and
>> > > > > > setups? If yes, there needs to be a discussion and agreement
>> that
>> > > this
>> > > > > > change is desirable. - Does the contribution conceptually fit
>> well
>> > > into
>> > > > > > Flink? Is it too much of special case such that it makes things
>> > more
>> > > > > > complicated for the common case, or bloats the abstractions /
>> > APIs? -
>> > > > > Does
>> > > > > > the feature fit well into Flink’s architecture? Will it scale
>> and
>> > > keep
>> > > > > > Flink flexible for the future, or will the feature restrict
>> Flink
>> > in
>> > > > the
>> > > > > > future? - Is the feature a significant new addition (rather
>> than an
>> > > > > > improvement to an existing part)? If yes, will the Flink
>> community
>> > > > commit
>> > > > > > to maintaining this feature? - Does the feature produce added
>> value
>> > > for
>> > > > > > Flink users or developers? Or does it introduce risk of
>> regression
>> > > > > without
>> > > > > > adding relevant user or developer benefit?All of these questions
>> > > should
>> > > > > be
>> > > > > > answerable from the description/discussion in JIRA and Pull
>> > Request,
>> > > > > > without looking at the code.(2) Does the contribution need
>> > attention
>> > > > from
>> > > > > > some specific committers and is there time commitment from these
>> > > > > > committers?Some changes require attention and approval from
>> > specific
>> > > > > > committers. For example, changes in parts that are either very
>> > > > > performance
>> > > > > > sensitive, or have a critical impact on distributed coordination
>> > and
>> > > > > fault
>> > > > > > tolerance need input by a committer that is deeply familiar with
>> > the
>> > > > > > component.As a rule of thumb, this is the case when the Pull
>> > Request
>> > > > > > description answers one of the questions in the template section
>> > > “Does
>> > > > > this
>> > > > > > pull request potentially affect one of the following parts” with
>> > > > > ‘yes’.This
>> > > > > > question can be answered with - Does not need specific
>> attention-
>> > > Needs
>> > > > > > specific attention for X (X can be for example checkpointing,
>> > > > jobmanager,
>> > > > > > etc.).- Has specific attention for X by @commiterA,
>> @contributorBIf
>> > > the
>> > > > > > pull request needs specific attention, one of the tagged
>> > > > > > committers/contributors should give the final approval.(3) Is
>> the
>> > > > > > contribution described well?Check whether the contribution is
>> > > > > sufficiently
>> > > > > > well described to support a good review. Trivial changes and
>> fixes
>> > do
>> > > > not
>> > > > > > need a long description. Any pull request that changes
>> > functionality
>> > > or
>> > > > > > behavior needs to describe the big picture of these changes, so
>> > that
>> > > > > > reviews know what to look for (and don’t have to dig through the
>> > code
>> > > > to
>> > > > > > hopefully understand what the change does).Changes that require
>> > > longer
>> > > > > > descriptions are ideally based on a prior design discussion in
>> the
>> > > > > mailing
>> > > > > > list or in JIRA and can simply link to there or copy the
>> > description
>> > > > from
>> > > > > > there.(4) Does the implementation follow the right overall
>> > > > > > approach/architecture?Is this the best approach to implement the
>> > fix
>> > > or
>> > > > > > feature, or are there other approaches that would be easier,
>> more
>> > > > robust,
>> > > > > > or more maintainable?This question should be answerable from the
>> > Pull
>> > > > > > Request description (or the linked JIRA) as much as possible.We
>> > > > recommend
>> > > > > > to check this before diving into the details of commenting on
>> > > > individual
>> > > > > > parts of the change.(5) Is the overall code quality good,
>> meeting
>> > > > > standard
>> > > > > > we want to maintain in Flink?This is the detailed code review of
>> > the
>> > > > > actual
>> > > > > > changes, covering: - Are the changes doing what is described in
>> the
>> > > > > design
>> > > > > > document or PR description?- Does the code follow the right
>> > software
>> > > > > > engineering practices? It the code correct, robust,
>> maintainable,
>> > > > > > testable?- Are the change performance aware, when changing a
>> > > > performance
>> > > > > > sensitive part?- Are the changes sufficiently covered by tests?-
>> > Are
>> > > > the
>> > > > > > tests executing fast?- Does the code format follow Flink’s
>> > checkstyle
>> > > > > > pattern?- Does the code avoid to introduce additional compiler
>> > > > > > warnings?Some code style guidelines can be found in the [Flink
>> Code
>> > > > Style
>> > > > > > Page](https://flink.apache.org/contribute-code.html#code-style
>> > > > > > <https://flink.apache.org/contribute-code.html#code-style>)Pull
>> > > > Request
>> > > > > > Review TemplateAdd the following checklist to the pull request
>> > > review,
>> > > > > > checking the boxes as the questions are answered:  - [ ]
>> Consensus
>> > > that
>> > > > > the
>> > > > > > contribution should go into to Flink  - [ ] Does not need
>> specific
>> > > > > > attention | Needs specific attention for X | Has attention for X
>> > by Y
>> > > > -
>> > > > > [
>> > > > > > ] Contribution description  - [ ] Architectural approach  - [ ]
>> > > Overall
>> > > > > > code quality*
>> > > > > >
>> > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

Piotr Nowojski
Hi,

I would like to rise one more issue. Often contributions are very heavy and difficult to review. They have one big commit that changes multiple things which is difficult to review. Instead I would be in favour of implementing a rule, that a single commit should never do more then one thing. For example never mixing refactoring/renames/clean ups/code deduplications with functional changes. If implementing something requires two independent changes in two separate components, those also should be two independent commits. In other words, if there are two changed lines in a single commit, they should interact somehow together and strictly depend on one another. This has couple of important advantages:

1. Obviously faster reviews. Especially if a reviewer do not have to find 2 lines bug fix among 200 lines of renames/refactoring.
2. Provides good “cut out” points for reviewer. For example he can easily interrupt reviewing in the middle and continue later or even merge PR in stages.
3. Better reference “why something was done this way not the other” for the future. This is the same argument as first point, however with benefit not during reviewing, but when after merging someone is trying to understand the code.
4. Commit message becomes much better place to write down reasons why something was done and what are the effects (not that this should replace comments/documentation, only to complement it).
5. In case of need to revert/drop some part of the contribution, we are not loosing all of it. If we have to revert some small feature, it would be easier to keep refactoring and clean ups.


Some examples of PRs that were more or less following this rule:
https://github.com/apache/flink/pull/6692/commits 
https://github.com/apache/flink/pull/5423/commits <https://github.com/apache/flink/pull/5423/commits> (a bit extreme)

If someone is not convinced I encourage to open those PRs and browse through couple of first commits (which are refactoring/clean up commits) one by one (GitHub has next/prev commit button). Then imagine if they were squashed with some functional/performance improvement changes.

Piotrek

> On 18 Sep 2018, at 17:12, 陈梓立 <[hidden email]> wrote:
>
> Put some good cases here might be helpful.
>
> See how a contribution of runtime module be proposed, discussed,
> implemented and merged from  https://github.com/apache/flink/pull/5931 to
> https://github.com/apache/flink/pull/6132.
>
> 1. #5931 fix a bug, but remains points could be improved. Here sihuazhou
> and shuai-xu share their considerations and require review(of the proposal)
> by Stephan, Till and Gary, our committers.
> 2. After discussion, all people involved reach a consensus. So the rest
> work is to implement it.
> 3. sihuazhou gives out an implementation #6132, Till reviews it and find it
> is somewhat out of the "architectural" aspect, so suggests
> implementation-level changes.
> 4. Addressing those implementation-level comments, the PR gets merged.
>
> I think this is quite a good example how we think our review process should
> go.
>
> Best,
> tison.
>
>
> 陈梓立 <[hidden email]> 于2018年9月18日周二 下午10:53写道:
>
>> Maybe a little rearrange to the process would help.
>>
>> (1). Does the contributor describe itself well?
>>  (1.1) By whom this contribution should be given attention. This often
>> shows by its title, "[FLINK-XXX] [module]", the module part infer.
>>  (1.2) What the purpose of this contribution is. Done by the PR template.
>> Even on JIRA an issue should cover these points.
>>
>> (2). Is there consensus on the contribution?
>> This follows (1), because we need to clear what the purpose of the
>> contribution first. At this stage reviewers could cc to module maintainer
>> as a supplement to (1.1). Also reviewers might ask the contributor to
>> clarify his purpose to sharp(1.2)
>>
>> (3). Is the implement architectural and fit code style?
>> This follows (2). And only after a consensus we talk about concrete
>> implement, which prevent spend time and put effort in vain.
>>
>> In addition, ideally a "+1" comment or approval means the purpose of
>> contribution is supported by the reviewer and implement(if there is)
>> quality is fine, so the reviewer vote for a consensus.
>>
>> Best,
>> tison.
>>
>>
>> Stephan Ewen <[hidden email]> 于2018年9月18日周二 下午6:44写道:
>>
>>> On the template discussion, some thoughts
>>>
>>> *PR Template*
>>>
>>> I think the PR template went well. We can rethink the "checklist" at the
>>> bottom, but all other parts turned out helpful in my opinion.
>>>
>>> With the amount of contributions, it helps to ask the contributor to take
>>> a
>>> little more work in order for the reviewer to be more efficient.
>>> I would suggest to keep that mindset: Whenever we find a way that the
>>> contributor can prepare stuff in such a way that reviews become
>>> more efficient, we should do that. In my experience, most contributors are
>>> willing to put in some extra minutes if it helps that their
>>> PR gets merged faster.
>>>
>>> *Review Template*
>>>
>>> I think it would be helpful to have this checklist. It does not matter in
>>> which form, be that as a text template, be that as labels.
>>>
>>> The most important thing is to make explicit which questions have been
>>> answered in the review.
>>> Currently there is a lot of "+1" on pull requests which means "code
>>> quality
>>> is fine", but all other questions are unanswered.
>>> The contributors then rightfully wonder why this does not get merged.
>>>
>>>
>>>
>>> On Tue, Sep 18, 2018 at 7:26 AM, 陈梓立 <[hidden email]> wrote:
>>>
>>>> Hi all interested,
>>>>
>>>> Within the document there is a heated discussion about how the PR
>>>> template/review template should be.
>>>>
>>>> Here share my opinion:
>>>>
>>>> 1. For the review template, actually we don't need comment a review
>>>> template at all. GitHub has a tag system and only committer could add
>>> tags,
>>>> which we can make use of it. That is, tagging this PR is
>>>> waiting-for-proposal-approved, waiting-for-code-review,
>>>> waiting-for-benchmark or block-by-author and so on. Asfbot could pick
>>>> GitHub tag state to the corresponding JIRA and we always regard JIRA as
>>> the
>>>> main discussion borad.
>>>>
>>>> 2. For the PR template, the greeting message is redundant. Just
>>> emphasize a
>>>> JIRA associated is important and how to format the title is enough.
>>>> Besides, the "Does this pull request potentially affect one of the
>>>> following parts" part and "Documentation" should be coved from "What is
>>> the
>>>> purpose of the change" and "Brief change log". These two parts, users
>>>> always answer no and would be aware if they really make changes on it.
>>> As
>>>> example, even pull request requires document, its owner might no add it
>>> at
>>>> first. The PR template is a guide but not which one have to learn.
>>>>
>>>> To sum up, (1) take advantage of GitHub's tag system to tag review
>>> progress
>>>> (2) make the template more concise to avoid burden mature contributors
>>> and
>>>> force new comer to learn too much.
>>>>
>>>> Best,
>>>> tison.
>>>>
>>>>
>>>> Rong Rong <[hidden email]> 于2018年9月18日周二 上午7:05写道:
>>>>
>>>>> Thanks for putting the review contribution doc together, Stephan! This
>>>> will
>>>>> definitely help the community to make the review process better.
>>>>>
>>>>> From my experience this will benefit on both contributors and
>>> reviewers
>>>>> side! Thus +1 for putting into practice as well.
>>>>>
>>>>> --
>>>>> Rong
>>>>>
>>>>> On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]>
>>> wrote:
>>>>>
>>>>>> Hi!
>>>>>>
>>>>>> Thanks you for the encouraging feedback so far.
>>>>>>
>>>>>> The overall goal is definitely to make the contribution process
>>> better
>>>>> and
>>>>>> get fewer pull requests that are disregarded.
>>>>>>
>>>>>> There are various reasons for the disregarded pull requests, one
>>> being
>>>>> that
>>>>>> fewer committers really participate in reviews beyond
>>>>>> the component they are currently very involved with. This is a
>>> separate
>>>>>> issue and I am thinking on how to encourage more
>>>>>> activity there.
>>>>>>
>>>>>> The other reason I was lack of structure and lack of decision
>>> making,
>>>>> which
>>>>>> is what I am first trying to fix here.
>>>>>> A follow-up to this will definitely be to improve the contribution
>>>> guide
>>>>> as
>>>>>> well.
>>>>>>
>>>>>> Best,
>>>>>> Stephan
>>>>>>
>>>>>>
>>>>>> On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
>>>>>> [hidden email]> wrote:
>>>>>>
>>>>>>> From my personal experience as a contributor for three years, I
>>> feel
>>>>>>> better experience in contirbuting or reviewing than before,
>>> although
>>>> we
>>>>>>> still have some points for further progress.
>>>>>>>
>>>>>>> I reviewed the proposal doc, and it gives very constructive and
>>>>>> meaningful
>>>>>>> guides which could help both contributor and reviewer. I agree
>>> with
>>>> the
>>>>>>> bove suggestions and wish they can be praticed well!
>>>>>>>
>>>>>>> Best,
>>>>>>> Zhijiang
>>>>>>> ------------------------------------------------------------------
>>>>>>> 发件人:Till Rohrmann <[hidden email]>
>>>>>>> 发送时间:2018年9月17日(星期一) 16:27
>>>>>>> 收件人:dev <[hidden email]>
>>>>>>> 主 题:Re: [PROPOSAL] [community] A more structured approach to
>>> reviews
>>>>> and
>>>>>>> contributions
>>>>>>>
>>>>>>> Thanks for writing this up Stephan. I like the steps and hope
>>> that it
>>>>>> will
>>>>>>> help the community to make the review process better. Thus, +1 for
>>>>>> putting
>>>>>>> your proposal to practice.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Till
>>>>>>>
>>>>>>> On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]>
>>>>> wrote:
>>>>>>>
>>>>>>>> Hi Flink community members!
>>>>>>>>
>>>>>>>> As many of you will have noticed, the Flink project activity has
>>>> gone
>>>>>> up
>>>>>>>> again quite a bit.
>>>>>>>> There are many more contributions, which is an absolutely great
>>>> thing
>>>>>> to
>>>>>>>> have :-)
>>>>>>>>
>>>>>>>> However, we see a continuously growing backlog of pull requests
>>> and
>>>>>> JIRA
>>>>>>>> issues.
>>>>>>>> To make sure the community will be able to handle the increased
>>>>>> volume, I
>>>>>>>> think we need to revisit some
>>>>>>>> approaches and processes. I believe there are a few
>>> opportunities
>>>> to
>>>>>>>> structure things a bit better, which
>>>>>>>> should help to scale the development.
>>>>>>>>
>>>>>>>> The first thing I would like to bring up are *Pull Request
>>>> Reviews*.
>>>>>> Even
>>>>>>>> though more community members being
>>>>>>>> active in reviews (which is a really great thing!) the Pull
>>> Request
>>>>>>> backlog
>>>>>>>> is increasing quite a bit.
>>>>>>>>
>>>>>>>> Why are pull requests still not merged faster? Looking at the
>>>>> reviews,
>>>>>>> one
>>>>>>>> thing I noticed is that most reviews deal
>>>>>>>> immediately with detailed code issues, and leave out most of the
>>>> core
>>>>>>>> questions that need to be answered
>>>>>>>> before a Pull Request can be merged, like "is this a desired
>>>>> feature?"
>>>>>> or
>>>>>>>> "does this align well with other developments?".
>>>>>>>> I think that we even make things slightly worse that way: From
>>> my
>>>>>>> personal
>>>>>>>> experience, I have often thought "oh, this
>>>>>>>> PR has a review already" and rather looked at another PR, only
>>> to
>>>>> find
>>>>>>>> later that the first review did never decide whether
>>>>>>>> this PR is actually a good fit for Flink.
>>>>>>>>
>>>>>>>> There has never been a proper documentation of how to answer
>>> these
>>>>>>>> questions, what to evaluate in reviews,
>>>>>>>> guidelines for how to evaluate pull requests, other than code
>>>>> quality.
>>>>>> I
>>>>>>>> suspect that this is why so many reviewers
>>>>>>>> do not address the "is this a good contribution" questions,
>>> making
>>>>> pull
>>>>>>>> requests linger until another committers joins
>>>>>>>> the review.
>>>>>>>>
>>>>>>>> Below is an idea for a guide *"How to Review Contributions"*. It
>>>>>> outlines
>>>>>>>> five core aspects to be checked in every
>>>>>>>> pull request, and suggests a priority for clarifying those. The
>>>> idea
>>>>> is
>>>>>>>> that this helps us to better structure reviews, and
>>>>>>>> to make each reviewer aware what we look for in a review and
>>> where
>>>> to
>>>>>>> best
>>>>>>>> bring in their help.
>>>>>>>>
>>>>>>>> Looking forward to comments!
>>>>>>>>
>>>>>>>> Best,
>>>>>>>> Stephan
>>>>>>>>
>>>>>>>> ====================================
>>>>>>>>
>>>>>>>> The draft is in this Google Doc. Please add small textual
>>> comments
>>>> to
>>>>>> the
>>>>>>>> doc, and bigger principle discussions as replies to this mail.
>>>>>>>>
>>>>>>>>
>>>>>>>> https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
>>>>>>> RbocGlGKCYnvJd9lVhk/edit?usp=sharing
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> *How to Review Contributions------------------------------This
>>>> guide
>>>>> is
>>>>>>> for
>>>>>>>> all committers and contributors that want to help with reviewing
>>>>>>>> contributions. Thank you for your effort - good reviews are one
>>> the
>>>>>> most
>>>>>>>> important and crucial parts of an open source project. This
>>> guide
>>>>>> should
>>>>>>>> help the community to make reviews such that: - Contributors
>>> have a
>>>>>> good
>>>>>>>> contribution experience- Reviews are structured and check all
>>>>> important
>>>>>>>> aspects of a contribution- Make sure we keep a high code
>>> quality in
>>>>>>> Flink-
>>>>>>>> We avoid situations where contributors and reviewers spend a
>>> lot of
>>>>>> time
>>>>>>> to
>>>>>>>> refine a contribution that gets rejected laterReview
>>> ChecklistEvery
>>>>>>> review
>>>>>>>> needs to check the following five aspects. We encourage to check
>>>>> these
>>>>>>>> aspects in order, to avoid spending time on detailed code
>>> quality
>>>>>> reviews
>>>>>>>> when there is not yet consensus that a feature or change should
>>> be
>>>>>>> actually
>>>>>>>> be added.(1) Is there consensus whether the change of feature
>>>> should
>>>>> go
>>>>>>>> into to Flink?For bug fixes, this needs to be checked only in
>>> case
>>>> it
>>>>>>>> requires bigger changes or might break existing programs and
>>>>>>>> setups.Ideally, this question is already answered from a JIRA
>>> issue
>>>>> or
>>>>>>> the
>>>>>>>> dev-list discussion, except in cases of bug fixes and small
>>>>> lightweight
>>>>>>>> additions/extensions. In that case, this question can be
>>>> immediately
>>>>>>> marked
>>>>>>>> as resolved. For pull requests that are created without prior
>>>>>> consensus,
>>>>>>>> this question needs to be answered as part of the review.The
>>>> decision
>>>>>>>> whether the change should go into Flink needs to take the
>>> following
>>>>>>> aspects
>>>>>>>> into consideration: - Does the contribution alter the behavior
>>> of
>>>>>>> features
>>>>>>>> or components in a way that it may break previous users’
>>> programs
>>>> and
>>>>>>>> setups? If yes, there needs to be a discussion and agreement
>>> that
>>>>> this
>>>>>>>> change is desirable. - Does the contribution conceptually fit
>>> well
>>>>> into
>>>>>>>> Flink? Is it too much of special case such that it makes things
>>>> more
>>>>>>>> complicated for the common case, or bloats the abstractions /
>>>> APIs? -
>>>>>>> Does
>>>>>>>> the feature fit well into Flink’s architecture? Will it scale
>>> and
>>>>> keep
>>>>>>>> Flink flexible for the future, or will the feature restrict
>>> Flink
>>>> in
>>>>>> the
>>>>>>>> future? - Is the feature a significant new addition (rather
>>> than an
>>>>>>>> improvement to an existing part)? If yes, will the Flink
>>> community
>>>>>> commit
>>>>>>>> to maintaining this feature? - Does the feature produce added
>>> value
>>>>> for
>>>>>>>> Flink users or developers? Or does it introduce risk of
>>> regression
>>>>>>> without
>>>>>>>> adding relevant user or developer benefit?All of these questions
>>>>> should
>>>>>>> be
>>>>>>>> answerable from the description/discussion in JIRA and Pull
>>>> Request,
>>>>>>>> without looking at the code.(2) Does the contribution need
>>>> attention
>>>>>> from
>>>>>>>> some specific committers and is there time commitment from these
>>>>>>>> committers?Some changes require attention and approval from
>>>> specific
>>>>>>>> committers. For example, changes in parts that are either very
>>>>>>> performance
>>>>>>>> sensitive, or have a critical impact on distributed coordination
>>>> and
>>>>>>> fault
>>>>>>>> tolerance need input by a committer that is deeply familiar with
>>>> the
>>>>>>>> component.As a rule of thumb, this is the case when the Pull
>>>> Request
>>>>>>>> description answers one of the questions in the template section
>>>>> “Does
>>>>>>> this
>>>>>>>> pull request potentially affect one of the following parts” with
>>>>>>> ‘yes’.This
>>>>>>>> question can be answered with - Does not need specific
>>> attention-
>>>>> Needs
>>>>>>>> specific attention for X (X can be for example checkpointing,
>>>>>> jobmanager,
>>>>>>>> etc.).- Has specific attention for X by @commiterA,
>>> @contributorBIf
>>>>> the
>>>>>>>> pull request needs specific attention, one of the tagged
>>>>>>>> committers/contributors should give the final approval.(3) Is
>>> the
>>>>>>>> contribution described well?Check whether the contribution is
>>>>>>> sufficiently
>>>>>>>> well described to support a good review. Trivial changes and
>>> fixes
>>>> do
>>>>>> not
>>>>>>>> need a long description. Any pull request that changes
>>>> functionality
>>>>> or
>>>>>>>> behavior needs to describe the big picture of these changes, so
>>>> that
>>>>>>>> reviews know what to look for (and don’t have to dig through the
>>>> code
>>>>>> to
>>>>>>>> hopefully understand what the change does).Changes that require
>>>>> longer
>>>>>>>> descriptions are ideally based on a prior design discussion in
>>> the
>>>>>>> mailing
>>>>>>>> list or in JIRA and can simply link to there or copy the
>>>> description
>>>>>> from
>>>>>>>> there.(4) Does the implementation follow the right overall
>>>>>>>> approach/architecture?Is this the best approach to implement the
>>>> fix
>>>>> or
>>>>>>>> feature, or are there other approaches that would be easier,
>>> more
>>>>>> robust,
>>>>>>>> or more maintainable?This question should be answerable from the
>>>> Pull
>>>>>>>> Request description (or the linked JIRA) as much as possible.We
>>>>>> recommend
>>>>>>>> to check this before diving into the details of commenting on
>>>>>> individual
>>>>>>>> parts of the change.(5) Is the overall code quality good,
>>> meeting
>>>>>>> standard
>>>>>>>> we want to maintain in Flink?This is the detailed code review of
>>>> the
>>>>>>> actual
>>>>>>>> changes, covering: - Are the changes doing what is described in
>>> the
>>>>>>> design
>>>>>>>> document or PR description?- Does the code follow the right
>>>> software
>>>>>>>> engineering practices? It the code correct, robust,
>>> maintainable,
>>>>>>>> testable?- Are the change performance aware, when changing a
>>>>>> performance
>>>>>>>> sensitive part?- Are the changes sufficiently covered by tests?-
>>>> Are
>>>>>> the
>>>>>>>> tests executing fast?- Does the code format follow Flink’s
>>>> checkstyle
>>>>>>>> pattern?- Does the code avoid to introduce additional compiler
>>>>>>>> warnings?Some code style guidelines can be found in the [Flink
>>> Code
>>>>>> Style
>>>>>>>> Page](https://flink.apache.org/contribute-code.html#code-style
>>>>>>>> <https://flink.apache.org/contribute-code.html#code-style>)Pull
>>>>>> Request
>>>>>>>> Review TemplateAdd the following checklist to the pull request
>>>>> review,
>>>>>>>> checking the boxes as the questions are answered:  - [ ]
>>> Consensus
>>>>> that
>>>>>>> the
>>>>>>>> contribution should go into to Flink  - [ ] Does not need
>>> specific
>>>>>>>> attention | Needs specific attention for X | Has attention for X
>>>> by Y
>>>>>> -
>>>>>>> [
>>>>>>>> ] Contribution description  - [ ] Architectural approach  - [ ]
>>>>> Overall
>>>>>>>> code quality*
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

tison
Hi Piotr,

I strongly agree with your idea. Since we work on a huge project, diff in
the PR becomes hard to review if there is too much noise.

Just to clarify the process. Do you mean that a pull request should go into
the way below?

Separated commits during the implement, mainly distinguish feature/bug fix
with clean up/rework.

[FLINK-XXX] [module] future/bug fix...
[FLINK-XXX] [module] more on future/bug fix...
[hotfix] clean up/rework...

and so on.

And finally, when get merged, put all stuff into one commit and comments
close #PRID
so coming ones could see the detail by jump to #PRID.

One thing to trade off is that if we merge by one commit, we cannot revert
part of it automated; if we merge by PR commits(one by one), the commit log
might mess.

Best,
tison.


Piotr Nowojski <[hidden email]> 于2018年9月19日周三 下午5:10写道:

> Hi,
>
> I would like to rise one more issue. Often contributions are very heavy
> and difficult to review. They have one big commit that changes multiple
> things which is difficult to review. Instead I would be in favour of
> implementing a rule, that a single commit should never do more then one
> thing. For example never mixing refactoring/renames/clean ups/code
> deduplications with functional changes. If implementing something requires
> two independent changes in two separate components, those also should be
> two independent commits. In other words, if there are two changed lines in
> a single commit, they should interact somehow together and strictly depend
> on one another. This has couple of important advantages:
>
> 1. Obviously faster reviews. Especially if a reviewer do not have to find
> 2 lines bug fix among 200 lines of renames/refactoring.
> 2. Provides good “cut out” points for reviewer. For example he can easily
> interrupt reviewing in the middle and continue later or even merge PR in
> stages.
> 3. Better reference “why something was done this way not the other” for
> the future. This is the same argument as first point, however with benefit
> not during reviewing, but when after merging someone is trying to
> understand the code.
> 4. Commit message becomes much better place to write down reasons why
> something was done and what are the effects (not that this should replace
> comments/documentation, only to complement it).
> 5. In case of need to revert/drop some part of the contribution, we are
> not loosing all of it. If we have to revert some small feature, it would be
> easier to keep refactoring and clean ups.
>
>
> Some examples of PRs that were more or less following this rule:
> https://github.com/apache/flink/pull/6692/commits
> https://github.com/apache/flink/pull/5423/commits <
> https://github.com/apache/flink/pull/5423/commits> (a bit extreme)
>
> If someone is not convinced I encourage to open those PRs and browse
> through couple of first commits (which are refactoring/clean up commits)
> one by one (GitHub has next/prev commit button). Then imagine if they were
> squashed with some functional/performance improvement changes.
>
> Piotrek
>
> > On 18 Sep 2018, at 17:12, 陈梓立 <[hidden email]> wrote:
> >
> > Put some good cases here might be helpful.
> >
> > See how a contribution of runtime module be proposed, discussed,
> > implemented and merged from  https://github.com/apache/flink/pull/5931
> to
> > https://github.com/apache/flink/pull/6132.
> >
> > 1. #5931 fix a bug, but remains points could be improved. Here sihuazhou
> > and shuai-xu share their considerations and require review(of the
> proposal)
> > by Stephan, Till and Gary, our committers.
> > 2. After discussion, all people involved reach a consensus. So the rest
> > work is to implement it.
> > 3. sihuazhou gives out an implementation #6132, Till reviews it and find
> it
> > is somewhat out of the "architectural" aspect, so suggests
> > implementation-level changes.
> > 4. Addressing those implementation-level comments, the PR gets merged.
> >
> > I think this is quite a good example how we think our review process
> should
> > go.
> >
> > Best,
> > tison.
> >
> >
> > 陈梓立 <[hidden email]> 于2018年9月18日周二 下午10:53写道:
> >
> >> Maybe a little rearrange to the process would help.
> >>
> >> (1). Does the contributor describe itself well?
> >>  (1.1) By whom this contribution should be given attention. This often
> >> shows by its title, "[FLINK-XXX] [module]", the module part infer.
> >>  (1.2) What the purpose of this contribution is. Done by the PR
> template.
> >> Even on JIRA an issue should cover these points.
> >>
> >> (2). Is there consensus on the contribution?
> >> This follows (1), because we need to clear what the purpose of the
> >> contribution first. At this stage reviewers could cc to module
> maintainer
> >> as a supplement to (1.1). Also reviewers might ask the contributor to
> >> clarify his purpose to sharp(1.2)
> >>
> >> (3). Is the implement architectural and fit code style?
> >> This follows (2). And only after a consensus we talk about concrete
> >> implement, which prevent spend time and put effort in vain.
> >>
> >> In addition, ideally a "+1" comment or approval means the purpose of
> >> contribution is supported by the reviewer and implement(if there is)
> >> quality is fine, so the reviewer vote for a consensus.
> >>
> >> Best,
> >> tison.
> >>
> >>
> >> Stephan Ewen <[hidden email]> 于2018年9月18日周二 下午6:44写道:
> >>
> >>> On the template discussion, some thoughts
> >>>
> >>> *PR Template*
> >>>
> >>> I think the PR template went well. We can rethink the "checklist" at
> the
> >>> bottom, but all other parts turned out helpful in my opinion.
> >>>
> >>> With the amount of contributions, it helps to ask the contributor to
> take
> >>> a
> >>> little more work in order for the reviewer to be more efficient.
> >>> I would suggest to keep that mindset: Whenever we find a way that the
> >>> contributor can prepare stuff in such a way that reviews become
> >>> more efficient, we should do that. In my experience, most contributors
> are
> >>> willing to put in some extra minutes if it helps that their
> >>> PR gets merged faster.
> >>>
> >>> *Review Template*
> >>>
> >>> I think it would be helpful to have this checklist. It does not matter
> in
> >>> which form, be that as a text template, be that as labels.
> >>>
> >>> The most important thing is to make explicit which questions have been
> >>> answered in the review.
> >>> Currently there is a lot of "+1" on pull requests which means "code
> >>> quality
> >>> is fine", but all other questions are unanswered.
> >>> The contributors then rightfully wonder why this does not get merged.
> >>>
> >>>
> >>>
> >>> On Tue, Sep 18, 2018 at 7:26 AM, 陈梓立 <[hidden email]> wrote:
> >>>
> >>>> Hi all interested,
> >>>>
> >>>> Within the document there is a heated discussion about how the PR
> >>>> template/review template should be.
> >>>>
> >>>> Here share my opinion:
> >>>>
> >>>> 1. For the review template, actually we don't need comment a review
> >>>> template at all. GitHub has a tag system and only committer could add
> >>> tags,
> >>>> which we can make use of it. That is, tagging this PR is
> >>>> waiting-for-proposal-approved, waiting-for-code-review,
> >>>> waiting-for-benchmark or block-by-author and so on. Asfbot could pick
> >>>> GitHub tag state to the corresponding JIRA and we always regard JIRA
> as
> >>> the
> >>>> main discussion borad.
> >>>>
> >>>> 2. For the PR template, the greeting message is redundant. Just
> >>> emphasize a
> >>>> JIRA associated is important and how to format the title is enough.
> >>>> Besides, the "Does this pull request potentially affect one of the
> >>>> following parts" part and "Documentation" should be coved from "What
> is
> >>> the
> >>>> purpose of the change" and "Brief change log". These two parts, users
> >>>> always answer no and would be aware if they really make changes on it.
> >>> As
> >>>> example, even pull request requires document, its owner might no add
> it
> >>> at
> >>>> first. The PR template is a guide but not which one have to learn.
> >>>>
> >>>> To sum up, (1) take advantage of GitHub's tag system to tag review
> >>> progress
> >>>> (2) make the template more concise to avoid burden mature contributors
> >>> and
> >>>> force new comer to learn too much.
> >>>>
> >>>> Best,
> >>>> tison.
> >>>>
> >>>>
> >>>> Rong Rong <[hidden email]> 于2018年9月18日周二 上午7:05写道:
> >>>>
> >>>>> Thanks for putting the review contribution doc together, Stephan!
> This
> >>>> will
> >>>>> definitely help the community to make the review process better.
> >>>>>
> >>>>> From my experience this will benefit on both contributors and
> >>> reviewers
> >>>>> side! Thus +1 for putting into practice as well.
> >>>>>
> >>>>> --
> >>>>> Rong
> >>>>>
> >>>>> On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]>
> >>> wrote:
> >>>>>
> >>>>>> Hi!
> >>>>>>
> >>>>>> Thanks you for the encouraging feedback so far.
> >>>>>>
> >>>>>> The overall goal is definitely to make the contribution process
> >>> better
> >>>>> and
> >>>>>> get fewer pull requests that are disregarded.
> >>>>>>
> >>>>>> There are various reasons for the disregarded pull requests, one
> >>> being
> >>>>> that
> >>>>>> fewer committers really participate in reviews beyond
> >>>>>> the component they are currently very involved with. This is a
> >>> separate
> >>>>>> issue and I am thinking on how to encourage more
> >>>>>> activity there.
> >>>>>>
> >>>>>> The other reason I was lack of structure and lack of decision
> >>> making,
> >>>>> which
> >>>>>> is what I am first trying to fix here.
> >>>>>> A follow-up to this will definitely be to improve the contribution
> >>>> guide
> >>>>> as
> >>>>>> well.
> >>>>>>
> >>>>>> Best,
> >>>>>> Stephan
> >>>>>>
> >>>>>>
> >>>>>> On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
> >>>>>> [hidden email]> wrote:
> >>>>>>
> >>>>>>> From my personal experience as a contributor for three years, I
> >>> feel
> >>>>>>> better experience in contirbuting or reviewing than before,
> >>> although
> >>>> we
> >>>>>>> still have some points for further progress.
> >>>>>>>
> >>>>>>> I reviewed the proposal doc, and it gives very constructive and
> >>>>>> meaningful
> >>>>>>> guides which could help both contributor and reviewer. I agree
> >>> with
> >>>> the
> >>>>>>> bove suggestions and wish they can be praticed well!
> >>>>>>>
> >>>>>>> Best,
> >>>>>>> Zhijiang
> >>>>>>> ------------------------------------------------------------------
> >>>>>>> 发件人:Till Rohrmann <[hidden email]>
> >>>>>>> 发送时间:2018年9月17日(星期一) 16:27
> >>>>>>> 收件人:dev <[hidden email]>
> >>>>>>> 主 题:Re: [PROPOSAL] [community] A more structured approach to
> >>> reviews
> >>>>> and
> >>>>>>> contributions
> >>>>>>>
> >>>>>>> Thanks for writing this up Stephan. I like the steps and hope
> >>> that it
> >>>>>> will
> >>>>>>> help the community to make the review process better. Thus, +1 for
> >>>>>> putting
> >>>>>>> your proposal to practice.
> >>>>>>>
> >>>>>>> Cheers,
> >>>>>>> Till
> >>>>>>>
> >>>>>>> On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]>
> >>>>> wrote:
> >>>>>>>
> >>>>>>>> Hi Flink community members!
> >>>>>>>>
> >>>>>>>> As many of you will have noticed, the Flink project activity has
> >>>> gone
> >>>>>> up
> >>>>>>>> again quite a bit.
> >>>>>>>> There are many more contributions, which is an absolutely great
> >>>> thing
> >>>>>> to
> >>>>>>>> have :-)
> >>>>>>>>
> >>>>>>>> However, we see a continuously growing backlog of pull requests
> >>> and
> >>>>>> JIRA
> >>>>>>>> issues.
> >>>>>>>> To make sure the community will be able to handle the increased
> >>>>>> volume, I
> >>>>>>>> think we need to revisit some
> >>>>>>>> approaches and processes. I believe there are a few
> >>> opportunities
> >>>> to
> >>>>>>>> structure things a bit better, which
> >>>>>>>> should help to scale the development.
> >>>>>>>>
> >>>>>>>> The first thing I would like to bring up are *Pull Request
> >>>> Reviews*.
> >>>>>> Even
> >>>>>>>> though more community members being
> >>>>>>>> active in reviews (which is a really great thing!) the Pull
> >>> Request
> >>>>>>> backlog
> >>>>>>>> is increasing quite a bit.
> >>>>>>>>
> >>>>>>>> Why are pull requests still not merged faster? Looking at the
> >>>>> reviews,
> >>>>>>> one
> >>>>>>>> thing I noticed is that most reviews deal
> >>>>>>>> immediately with detailed code issues, and leave out most of the
> >>>> core
> >>>>>>>> questions that need to be answered
> >>>>>>>> before a Pull Request can be merged, like "is this a desired
> >>>>> feature?"
> >>>>>> or
> >>>>>>>> "does this align well with other developments?".
> >>>>>>>> I think that we even make things slightly worse that way: From
> >>> my
> >>>>>>> personal
> >>>>>>>> experience, I have often thought "oh, this
> >>>>>>>> PR has a review already" and rather looked at another PR, only
> >>> to
> >>>>> find
> >>>>>>>> later that the first review did never decide whether
> >>>>>>>> this PR is actually a good fit for Flink.
> >>>>>>>>
> >>>>>>>> There has never been a proper documentation of how to answer
> >>> these
> >>>>>>>> questions, what to evaluate in reviews,
> >>>>>>>> guidelines for how to evaluate pull requests, other than code
> >>>>> quality.
> >>>>>> I
> >>>>>>>> suspect that this is why so many reviewers
> >>>>>>>> do not address the "is this a good contribution" questions,
> >>> making
> >>>>> pull
> >>>>>>>> requests linger until another committers joins
> >>>>>>>> the review.
> >>>>>>>>
> >>>>>>>> Below is an idea for a guide *"How to Review Contributions"*. It
> >>>>>> outlines
> >>>>>>>> five core aspects to be checked in every
> >>>>>>>> pull request, and suggests a priority for clarifying those. The
> >>>> idea
> >>>>> is
> >>>>>>>> that this helps us to better structure reviews, and
> >>>>>>>> to make each reviewer aware what we look for in a review and
> >>> where
> >>>> to
> >>>>>>> best
> >>>>>>>> bring in their help.
> >>>>>>>>
> >>>>>>>> Looking forward to comments!
> >>>>>>>>
> >>>>>>>> Best,
> >>>>>>>> Stephan
> >>>>>>>>
> >>>>>>>> ====================================
> >>>>>>>>
> >>>>>>>> The draft is in this Google Doc. Please add small textual
> >>> comments
> >>>> to
> >>>>>> the
> >>>>>>>> doc, and bigger principle discussions as replies to this mail.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
> >>>>>>> RbocGlGKCYnvJd9lVhk/edit?usp=sharing
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> *How to Review Contributions------------------------------This
> >>>> guide
> >>>>> is
> >>>>>>> for
> >>>>>>>> all committers and contributors that want to help with reviewing
> >>>>>>>> contributions. Thank you for your effort - good reviews are one
> >>> the
> >>>>>> most
> >>>>>>>> important and crucial parts of an open source project. This
> >>> guide
> >>>>>> should
> >>>>>>>> help the community to make reviews such that: - Contributors
> >>> have a
> >>>>>> good
> >>>>>>>> contribution experience- Reviews are structured and check all
> >>>>> important
> >>>>>>>> aspects of a contribution- Make sure we keep a high code
> >>> quality in
> >>>>>>> Flink-
> >>>>>>>> We avoid situations where contributors and reviewers spend a
> >>> lot of
> >>>>>> time
> >>>>>>> to
> >>>>>>>> refine a contribution that gets rejected laterReview
> >>> ChecklistEvery
> >>>>>>> review
> >>>>>>>> needs to check the following five aspects. We encourage to check
> >>>>> these
> >>>>>>>> aspects in order, to avoid spending time on detailed code
> >>> quality
> >>>>>> reviews
> >>>>>>>> when there is not yet consensus that a feature or change should
> >>> be
> >>>>>>> actually
> >>>>>>>> be added.(1) Is there consensus whether the change of feature
> >>>> should
> >>>>> go
> >>>>>>>> into to Flink?For bug fixes, this needs to be checked only in
> >>> case
> >>>> it
> >>>>>>>> requires bigger changes or might break existing programs and
> >>>>>>>> setups.Ideally, this question is already answered from a JIRA
> >>> issue
> >>>>> or
> >>>>>>> the
> >>>>>>>> dev-list discussion, except in cases of bug fixes and small
> >>>>> lightweight
> >>>>>>>> additions/extensions. In that case, this question can be
> >>>> immediately
> >>>>>>> marked
> >>>>>>>> as resolved. For pull requests that are created without prior
> >>>>>> consensus,
> >>>>>>>> this question needs to be answered as part of the review.The
> >>>> decision
> >>>>>>>> whether the change should go into Flink needs to take the
> >>> following
> >>>>>>> aspects
> >>>>>>>> into consideration: - Does the contribution alter the behavior
> >>> of
> >>>>>>> features
> >>>>>>>> or components in a way that it may break previous users’
> >>> programs
> >>>> and
> >>>>>>>> setups? If yes, there needs to be a discussion and agreement
> >>> that
> >>>>> this
> >>>>>>>> change is desirable. - Does the contribution conceptually fit
> >>> well
> >>>>> into
> >>>>>>>> Flink? Is it too much of special case such that it makes things
> >>>> more
> >>>>>>>> complicated for the common case, or bloats the abstractions /
> >>>> APIs? -
> >>>>>>> Does
> >>>>>>>> the feature fit well into Flink’s architecture? Will it scale
> >>> and
> >>>>> keep
> >>>>>>>> Flink flexible for the future, or will the feature restrict
> >>> Flink
> >>>> in
> >>>>>> the
> >>>>>>>> future? - Is the feature a significant new addition (rather
> >>> than an
> >>>>>>>> improvement to an existing part)? If yes, will the Flink
> >>> community
> >>>>>> commit
> >>>>>>>> to maintaining this feature? - Does the feature produce added
> >>> value
> >>>>> for
> >>>>>>>> Flink users or developers? Or does it introduce risk of
> >>> regression
> >>>>>>> without
> >>>>>>>> adding relevant user or developer benefit?All of these questions
> >>>>> should
> >>>>>>> be
> >>>>>>>> answerable from the description/discussion in JIRA and Pull
> >>>> Request,
> >>>>>>>> without looking at the code.(2) Does the contribution need
> >>>> attention
> >>>>>> from
> >>>>>>>> some specific committers and is there time commitment from these
> >>>>>>>> committers?Some changes require attention and approval from
> >>>> specific
> >>>>>>>> committers. For example, changes in parts that are either very
> >>>>>>> performance
> >>>>>>>> sensitive, or have a critical impact on distributed coordination
> >>>> and
> >>>>>>> fault
> >>>>>>>> tolerance need input by a committer that is deeply familiar with
> >>>> the
> >>>>>>>> component.As a rule of thumb, this is the case when the Pull
> >>>> Request
> >>>>>>>> description answers one of the questions in the template section
> >>>>> “Does
> >>>>>>> this
> >>>>>>>> pull request potentially affect one of the following parts” with
> >>>>>>> ‘yes’.This
> >>>>>>>> question can be answered with - Does not need specific
> >>> attention-
> >>>>> Needs
> >>>>>>>> specific attention for X (X can be for example checkpointing,
> >>>>>> jobmanager,
> >>>>>>>> etc.).- Has specific attention for X by @commiterA,
> >>> @contributorBIf
> >>>>> the
> >>>>>>>> pull request needs specific attention, one of the tagged
> >>>>>>>> committers/contributors should give the final approval.(3) Is
> >>> the
> >>>>>>>> contribution described well?Check whether the contribution is
> >>>>>>> sufficiently
> >>>>>>>> well described to support a good review. Trivial changes and
> >>> fixes
> >>>> do
> >>>>>> not
> >>>>>>>> need a long description. Any pull request that changes
> >>>> functionality
> >>>>> or
> >>>>>>>> behavior needs to describe the big picture of these changes, so
> >>>> that
> >>>>>>>> reviews know what to look for (and don’t have to dig through the
> >>>> code
> >>>>>> to
> >>>>>>>> hopefully understand what the change does).Changes that require
> >>>>> longer
> >>>>>>>> descriptions are ideally based on a prior design discussion in
> >>> the
> >>>>>>> mailing
> >>>>>>>> list or in JIRA and can simply link to there or copy the
> >>>> description
> >>>>>> from
> >>>>>>>> there.(4) Does the implementation follow the right overall
> >>>>>>>> approach/architecture?Is this the best approach to implement the
> >>>> fix
> >>>>> or
> >>>>>>>> feature, or are there other approaches that would be easier,
> >>> more
> >>>>>> robust,
> >>>>>>>> or more maintainable?This question should be answerable from the
> >>>> Pull
> >>>>>>>> Request description (or the linked JIRA) as much as possible.We
> >>>>>> recommend
> >>>>>>>> to check this before diving into the details of commenting on
> >>>>>> individual
> >>>>>>>> parts of the change.(5) Is the overall code quality good,
> >>> meeting
> >>>>>>> standard
> >>>>>>>> we want to maintain in Flink?This is the detailed code review of
> >>>> the
> >>>>>>> actual
> >>>>>>>> changes, covering: - Are the changes doing what is described in
> >>> the
> >>>>>>> design
> >>>>>>>> document or PR description?- Does the code follow the right
> >>>> software
> >>>>>>>> engineering practices? It the code correct, robust,
> >>> maintainable,
> >>>>>>>> testable?- Are the change performance aware, when changing a
> >>>>>> performance
> >>>>>>>> sensitive part?- Are the changes sufficiently covered by tests?-
> >>>> Are
> >>>>>> the
> >>>>>>>> tests executing fast?- Does the code format follow Flink’s
> >>>> checkstyle
> >>>>>>>> pattern?- Does the code avoid to introduce additional compiler
> >>>>>>>> warnings?Some code style guidelines can be found in the [Flink
> >>> Code
> >>>>>> Style
> >>>>>>>> Page](https://flink.apache.org/contribute-code.html#code-style
> >>>>>>>> <https://flink.apache.org/contribute-code.html#code-style>)Pull
> >>>>>> Request
> >>>>>>>> Review TemplateAdd the following checklist to the pull request
> >>>>> review,
> >>>>>>>> checking the boxes as the questions are answered:  - [ ]
> >>> Consensus
> >>>>> that
> >>>>>>> the
> >>>>>>>> contribution should go into to Flink  - [ ] Does not need
> >>> specific
> >>>>>>>> attention | Needs specific attention for X | Has attention for X
> >>>> by Y
> >>>>>> -
> >>>>>>> [
> >>>>>>>> ] Contribution description  - [ ] Architectural approach  - [ ]
> >>>>> Overall
> >>>>>>>> code quality*
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

Fabian Hueske-2
Hi,

I'd like to suggest that we keep this thread focused on discussing
Stephan's proposal, i.e., introducing a structured PR review process.
Tison and Piotr raised some good points related to PR reviews that are
definitely worth discussing, but I think we should do that on different
threads and move forward with the original discussion.

Stephan's proposal has received only positive feedback until now.
So, I think we should go ahead and adopt the process.

IMO, there are two things to be done here:

* Add the proposed process to the website (trivial)
* Implement the process in practice which is still an open question to me.

Two methods have been proposed to implement the process and I see following
dis/advantages:

1. Automatically posting the review checklist as the first comment to a PR
and track the progress by ticking off boxes in the checklist.

  + First time contributors / reviewers learn about the process from the
comment, which reduces the chance of detailed reviews without consensus on
the motivation and approach.
  - Comments are not visible when looking at the PR list and cannot be used
for filtering.
  - Needs some kind of service that automatically comments on PRs. Service
does not need special permissions as every GH user can comment.

2. Tracking the review status of a PR with labels.

  + Labels are visible on the PR overview and can be used to filter PRs.
  - The review process is not spelled out. Contributors and reviewers have
to learn about the process somewhere else (link could be added to PR
template).
  - If we want to tick-off labels, we needs some kind of service to
automatically assign labels. The service needs committer permissions or be
setup by ASF Infra.
  - We need to check if assignment/removal of labels is mirrored to a
mailing list which is important in the ASF to track decision. This
shouldn't be hard to figure out, but if labels are not tracked, they cannot
be the sole solution.

We can of course also do both.
Have the review checklist posted and tracked as a comment and ask reviewers
to add the right label when ticking a box off.

What do you think?

Best,
Fabian

2018-09-19 11:24 GMT+02:00 陈梓立 <[hidden email]>:

> Hi Piotr,
>
> I strongly agree with your idea. Since we work on a huge project, diff in
> the PR becomes hard to review if there is too much noise.
>
> Just to clarify the process. Do you mean that a pull request should go into
> the way below?
>
> Separated commits during the implement, mainly distinguish feature/bug fix
> with clean up/rework.
>
> [FLINK-XXX] [module] future/bug fix...
> [FLINK-XXX] [module] more on future/bug fix...
> [hotfix] clean up/rework...
>
> and so on.
>
> And finally, when get merged, put all stuff into one commit and comments
> close #PRID
> so coming ones could see the detail by jump to #PRID.
>
> One thing to trade off is that if we merge by one commit, we cannot revert
> part of it automated; if we merge by PR commits(one by one), the commit log
> might mess.
>
> Best,
> tison.
>
>
> Piotr Nowojski <[hidden email]> 于2018年9月19日周三 下午5:10写道:
>
> > Hi,
> >
> > I would like to rise one more issue. Often contributions are very heavy
> > and difficult to review. They have one big commit that changes multiple
> > things which is difficult to review. Instead I would be in favour of
> > implementing a rule, that a single commit should never do more then one
> > thing. For example never mixing refactoring/renames/clean ups/code
> > deduplications with functional changes. If implementing something
> requires
> > two independent changes in two separate components, those also should be
> > two independent commits. In other words, if there are two changed lines
> in
> > a single commit, they should interact somehow together and strictly
> depend
> > on one another. This has couple of important advantages:
> >
> > 1. Obviously faster reviews. Especially if a reviewer do not have to find
> > 2 lines bug fix among 200 lines of renames/refactoring.
> > 2. Provides good “cut out” points for reviewer. For example he can easily
> > interrupt reviewing in the middle and continue later or even merge PR in
> > stages.
> > 3. Better reference “why something was done this way not the other” for
> > the future. This is the same argument as first point, however with
> benefit
> > not during reviewing, but when after merging someone is trying to
> > understand the code.
> > 4. Commit message becomes much better place to write down reasons why
> > something was done and what are the effects (not that this should replace
> > comments/documentation, only to complement it).
> > 5. In case of need to revert/drop some part of the contribution, we are
> > not loosing all of it. If we have to revert some small feature, it would
> be
> > easier to keep refactoring and clean ups.
> >
> >
> > Some examples of PRs that were more or less following this rule:
> > https://github.com/apache/flink/pull/6692/commits
> > https://github.com/apache/flink/pull/5423/commits <
> > https://github.com/apache/flink/pull/5423/commits> (a bit extreme)
> >
> > If someone is not convinced I encourage to open those PRs and browse
> > through couple of first commits (which are refactoring/clean up commits)
> > one by one (GitHub has next/prev commit button). Then imagine if they
> were
> > squashed with some functional/performance improvement changes.
> >
> > Piotrek
> >
> > > On 18 Sep 2018, at 17:12, 陈梓立 <[hidden email]> wrote:
> > >
> > > Put some good cases here might be helpful.
> > >
> > > See how a contribution of runtime module be proposed, discussed,
> > > implemented and merged from  https://github.com/apache/flink/pull/5931
> > to
> > > https://github.com/apache/flink/pull/6132.
> > >
> > > 1. #5931 fix a bug, but remains points could be improved. Here
> sihuazhou
> > > and shuai-xu share their considerations and require review(of the
> > proposal)
> > > by Stephan, Till and Gary, our committers.
> > > 2. After discussion, all people involved reach a consensus. So the rest
> > > work is to implement it.
> > > 3. sihuazhou gives out an implementation #6132, Till reviews it and
> find
> > it
> > > is somewhat out of the "architectural" aspect, so suggests
> > > implementation-level changes.
> > > 4. Addressing those implementation-level comments, the PR gets merged.
> > >
> > > I think this is quite a good example how we think our review process
> > should
> > > go.
> > >
> > > Best,
> > > tison.
> > >
> > >
> > > 陈梓立 <[hidden email]> 于2018年9月18日周二 下午10:53写道:
> > >
> > >> Maybe a little rearrange to the process would help.
> > >>
> > >> (1). Does the contributor describe itself well?
> > >>  (1.1) By whom this contribution should be given attention. This often
> > >> shows by its title, "[FLINK-XXX] [module]", the module part infer.
> > >>  (1.2) What the purpose of this contribution is. Done by the PR
> > template.
> > >> Even on JIRA an issue should cover these points.
> > >>
> > >> (2). Is there consensus on the contribution?
> > >> This follows (1), because we need to clear what the purpose of the
> > >> contribution first. At this stage reviewers could cc to module
> > maintainer
> > >> as a supplement to (1.1). Also reviewers might ask the contributor to
> > >> clarify his purpose to sharp(1.2)
> > >>
> > >> (3). Is the implement architectural and fit code style?
> > >> This follows (2). And only after a consensus we talk about concrete
> > >> implement, which prevent spend time and put effort in vain.
> > >>
> > >> In addition, ideally a "+1" comment or approval means the purpose of
> > >> contribution is supported by the reviewer and implement(if there is)
> > >> quality is fine, so the reviewer vote for a consensus.
> > >>
> > >> Best,
> > >> tison.
> > >>
> > >>
> > >> Stephan Ewen <[hidden email]> 于2018年9月18日周二 下午6:44写道:
> > >>
> > >>> On the template discussion, some thoughts
> > >>>
> > >>> *PR Template*
> > >>>
> > >>> I think the PR template went well. We can rethink the "checklist" at
> > the
> > >>> bottom, but all other parts turned out helpful in my opinion.
> > >>>
> > >>> With the amount of contributions, it helps to ask the contributor to
> > take
> > >>> a
> > >>> little more work in order for the reviewer to be more efficient.
> > >>> I would suggest to keep that mindset: Whenever we find a way that the
> > >>> contributor can prepare stuff in such a way that reviews become
> > >>> more efficient, we should do that. In my experience, most
> contributors
> > are
> > >>> willing to put in some extra minutes if it helps that their
> > >>> PR gets merged faster.
> > >>>
> > >>> *Review Template*
> > >>>
> > >>> I think it would be helpful to have this checklist. It does not
> matter
> > in
> > >>> which form, be that as a text template, be that as labels.
> > >>>
> > >>> The most important thing is to make explicit which questions have
> been
> > >>> answered in the review.
> > >>> Currently there is a lot of "+1" on pull requests which means "code
> > >>> quality
> > >>> is fine", but all other questions are unanswered.
> > >>> The contributors then rightfully wonder why this does not get merged.
> > >>>
> > >>>
> > >>>
> > >>> On Tue, Sep 18, 2018 at 7:26 AM, 陈梓立 <[hidden email]> wrote:
> > >>>
> > >>>> Hi all interested,
> > >>>>
> > >>>> Within the document there is a heated discussion about how the PR
> > >>>> template/review template should be.
> > >>>>
> > >>>> Here share my opinion:
> > >>>>
> > >>>> 1. For the review template, actually we don't need comment a review
> > >>>> template at all. GitHub has a tag system and only committer could
> add
> > >>> tags,
> > >>>> which we can make use of it. That is, tagging this PR is
> > >>>> waiting-for-proposal-approved, waiting-for-code-review,
> > >>>> waiting-for-benchmark or block-by-author and so on. Asfbot could
> pick
> > >>>> GitHub tag state to the corresponding JIRA and we always regard JIRA
> > as
> > >>> the
> > >>>> main discussion borad.
> > >>>>
> > >>>> 2. For the PR template, the greeting message is redundant. Just
> > >>> emphasize a
> > >>>> JIRA associated is important and how to format the title is enough.
> > >>>> Besides, the "Does this pull request potentially affect one of the
> > >>>> following parts" part and "Documentation" should be coved from "What
> > is
> > >>> the
> > >>>> purpose of the change" and "Brief change log". These two parts,
> users
> > >>>> always answer no and would be aware if they really make changes on
> it.
> > >>> As
> > >>>> example, even pull request requires document, its owner might no add
> > it
> > >>> at
> > >>>> first. The PR template is a guide but not which one have to learn.
> > >>>>
> > >>>> To sum up, (1) take advantage of GitHub's tag system to tag review
> > >>> progress
> > >>>> (2) make the template more concise to avoid burden mature
> contributors
> > >>> and
> > >>>> force new comer to learn too much.
> > >>>>
> > >>>> Best,
> > >>>> tison.
> > >>>>
> > >>>>
> > >>>> Rong Rong <[hidden email]> 于2018年9月18日周二 上午7:05写道:
> > >>>>
> > >>>>> Thanks for putting the review contribution doc together, Stephan!
> > This
> > >>>> will
> > >>>>> definitely help the community to make the review process better.
> > >>>>>
> > >>>>> From my experience this will benefit on both contributors and
> > >>> reviewers
> > >>>>> side! Thus +1 for putting into practice as well.
> > >>>>>
> > >>>>> --
> > >>>>> Rong
> > >>>>>
> > >>>>> On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]>
> > >>> wrote:
> > >>>>>
> > >>>>>> Hi!
> > >>>>>>
> > >>>>>> Thanks you for the encouraging feedback so far.
> > >>>>>>
> > >>>>>> The overall goal is definitely to make the contribution process
> > >>> better
> > >>>>> and
> > >>>>>> get fewer pull requests that are disregarded.
> > >>>>>>
> > >>>>>> There are various reasons for the disregarded pull requests, one
> > >>> being
> > >>>>> that
> > >>>>>> fewer committers really participate in reviews beyond
> > >>>>>> the component they are currently very involved with. This is a
> > >>> separate
> > >>>>>> issue and I am thinking on how to encourage more
> > >>>>>> activity there.
> > >>>>>>
> > >>>>>> The other reason I was lack of structure and lack of decision
> > >>> making,
> > >>>>> which
> > >>>>>> is what I am first trying to fix here.
> > >>>>>> A follow-up to this will definitely be to improve the contribution
> > >>>> guide
> > >>>>> as
> > >>>>>> well.
> > >>>>>>
> > >>>>>> Best,
> > >>>>>> Stephan
> > >>>>>>
> > >>>>>>
> > >>>>>> On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
> > >>>>>> [hidden email]> wrote:
> > >>>>>>
> > >>>>>>> From my personal experience as a contributor for three years, I
> > >>> feel
> > >>>>>>> better experience in contirbuting or reviewing than before,
> > >>> although
> > >>>> we
> > >>>>>>> still have some points for further progress.
> > >>>>>>>
> > >>>>>>> I reviewed the proposal doc, and it gives very constructive and
> > >>>>>> meaningful
> > >>>>>>> guides which could help both contributor and reviewer. I agree
> > >>> with
> > >>>> the
> > >>>>>>> bove suggestions and wish they can be praticed well!
> > >>>>>>>
> > >>>>>>> Best,
> > >>>>>>> Zhijiang
> > >>>>>>> ------------------------------------------------------------
> ------
> > >>>>>>> 发件人:Till Rohrmann <[hidden email]>
> > >>>>>>> 发送时间:2018年9月17日(星期一) 16:27
> > >>>>>>> 收件人:dev <[hidden email]>
> > >>>>>>> 主 题:Re: [PROPOSAL] [community] A more structured approach to
> > >>> reviews
> > >>>>> and
> > >>>>>>> contributions
> > >>>>>>>
> > >>>>>>> Thanks for writing this up Stephan. I like the steps and hope
> > >>> that it
> > >>>>>> will
> > >>>>>>> help the community to make the review process better. Thus, +1
> for
> > >>>>>> putting
> > >>>>>>> your proposal to practice.
> > >>>>>>>
> > >>>>>>> Cheers,
> > >>>>>>> Till
> > >>>>>>>
> > >>>>>>> On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]>
> > >>>>> wrote:
> > >>>>>>>
> > >>>>>>>> Hi Flink community members!
> > >>>>>>>>
> > >>>>>>>> As many of you will have noticed, the Flink project activity has
> > >>>> gone
> > >>>>>> up
> > >>>>>>>> again quite a bit.
> > >>>>>>>> There are many more contributions, which is an absolutely great
> > >>>> thing
> > >>>>>> to
> > >>>>>>>> have :-)
> > >>>>>>>>
> > >>>>>>>> However, we see a continuously growing backlog of pull requests
> > >>> and
> > >>>>>> JIRA
> > >>>>>>>> issues.
> > >>>>>>>> To make sure the community will be able to handle the increased
> > >>>>>> volume, I
> > >>>>>>>> think we need to revisit some
> > >>>>>>>> approaches and processes. I believe there are a few
> > >>> opportunities
> > >>>> to
> > >>>>>>>> structure things a bit better, which
> > >>>>>>>> should help to scale the development.
> > >>>>>>>>
> > >>>>>>>> The first thing I would like to bring up are *Pull Request
> > >>>> Reviews*.
> > >>>>>> Even
> > >>>>>>>> though more community members being
> > >>>>>>>> active in reviews (which is a really great thing!) the Pull
> > >>> Request
> > >>>>>>> backlog
> > >>>>>>>> is increasing quite a bit.
> > >>>>>>>>
> > >>>>>>>> Why are pull requests still not merged faster? Looking at the
> > >>>>> reviews,
> > >>>>>>> one
> > >>>>>>>> thing I noticed is that most reviews deal
> > >>>>>>>> immediately with detailed code issues, and leave out most of the
> > >>>> core
> > >>>>>>>> questions that need to be answered
> > >>>>>>>> before a Pull Request can be merged, like "is this a desired
> > >>>>> feature?"
> > >>>>>> or
> > >>>>>>>> "does this align well with other developments?".
> > >>>>>>>> I think that we even make things slightly worse that way: From
> > >>> my
> > >>>>>>> personal
> > >>>>>>>> experience, I have often thought "oh, this
> > >>>>>>>> PR has a review already" and rather looked at another PR, only
> > >>> to
> > >>>>> find
> > >>>>>>>> later that the first review did never decide whether
> > >>>>>>>> this PR is actually a good fit for Flink.
> > >>>>>>>>
> > >>>>>>>> There has never been a proper documentation of how to answer
> > >>> these
> > >>>>>>>> questions, what to evaluate in reviews,
> > >>>>>>>> guidelines for how to evaluate pull requests, other than code
> > >>>>> quality.
> > >>>>>> I
> > >>>>>>>> suspect that this is why so many reviewers
> > >>>>>>>> do not address the "is this a good contribution" questions,
> > >>> making
> > >>>>> pull
> > >>>>>>>> requests linger until another committers joins
> > >>>>>>>> the review.
> > >>>>>>>>
> > >>>>>>>> Below is an idea for a guide *"How to Review Contributions"*. It
> > >>>>>> outlines
> > >>>>>>>> five core aspects to be checked in every
> > >>>>>>>> pull request, and suggests a priority for clarifying those. The
> > >>>> idea
> > >>>>> is
> > >>>>>>>> that this helps us to better structure reviews, and
> > >>>>>>>> to make each reviewer aware what we look for in a review and
> > >>> where
> > >>>> to
> > >>>>>>> best
> > >>>>>>>> bring in their help.
> > >>>>>>>>
> > >>>>>>>> Looking forward to comments!
> > >>>>>>>>
> > >>>>>>>> Best,
> > >>>>>>>> Stephan
> > >>>>>>>>
> > >>>>>>>> ====================================
> > >>>>>>>>
> > >>>>>>>> The draft is in this Google Doc. Please add small textual
> > >>> comments
> > >>>> to
> > >>>>>> the
> > >>>>>>>> doc, and bigger principle discussions as replies to this mail.
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
> > >>>>>>> RbocGlGKCYnvJd9lVhk/edit?usp=sharing
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> *How to Review Contributions------------------------------This
> > >>>> guide
> > >>>>> is
> > >>>>>>> for
> > >>>>>>>> all committers and contributors that want to help with reviewing
> > >>>>>>>> contributions. Thank you for your effort - good reviews are one
> > >>> the
> > >>>>>> most
> > >>>>>>>> important and crucial parts of an open source project. This
> > >>> guide
> > >>>>>> should
> > >>>>>>>> help the community to make reviews such that: - Contributors
> > >>> have a
> > >>>>>> good
> > >>>>>>>> contribution experience- Reviews are structured and check all
> > >>>>> important
> > >>>>>>>> aspects of a contribution- Make sure we keep a high code
> > >>> quality in
> > >>>>>>> Flink-
> > >>>>>>>> We avoid situations where contributors and reviewers spend a
> > >>> lot of
> > >>>>>> time
> > >>>>>>> to
> > >>>>>>>> refine a contribution that gets rejected laterReview
> > >>> ChecklistEvery
> > >>>>>>> review
> > >>>>>>>> needs to check the following five aspects. We encourage to check
> > >>>>> these
> > >>>>>>>> aspects in order, to avoid spending time on detailed code
> > >>> quality
> > >>>>>> reviews
> > >>>>>>>> when there is not yet consensus that a feature or change should
> > >>> be
> > >>>>>>> actually
> > >>>>>>>> be added.(1) Is there consensus whether the change of feature
> > >>>> should
> > >>>>> go
> > >>>>>>>> into to Flink?For bug fixes, this needs to be checked only in
> > >>> case
> > >>>> it
> > >>>>>>>> requires bigger changes or might break existing programs and
> > >>>>>>>> setups.Ideally, this question is already answered from a JIRA
> > >>> issue
> > >>>>> or
> > >>>>>>> the
> > >>>>>>>> dev-list discussion, except in cases of bug fixes and small
> > >>>>> lightweight
> > >>>>>>>> additions/extensions. In that case, this question can be
> > >>>> immediately
> > >>>>>>> marked
> > >>>>>>>> as resolved. For pull requests that are created without prior
> > >>>>>> consensus,
> > >>>>>>>> this question needs to be answered as part of the review.The
> > >>>> decision
> > >>>>>>>> whether the change should go into Flink needs to take the
> > >>> following
> > >>>>>>> aspects
> > >>>>>>>> into consideration: - Does the contribution alter the behavior
> > >>> of
> > >>>>>>> features
> > >>>>>>>> or components in a way that it may break previous users’
> > >>> programs
> > >>>> and
> > >>>>>>>> setups? If yes, there needs to be a discussion and agreement
> > >>> that
> > >>>>> this
> > >>>>>>>> change is desirable. - Does the contribution conceptually fit
> > >>> well
> > >>>>> into
> > >>>>>>>> Flink? Is it too much of special case such that it makes things
> > >>>> more
> > >>>>>>>> complicated for the common case, or bloats the abstractions /
> > >>>> APIs? -
> > >>>>>>> Does
> > >>>>>>>> the feature fit well into Flink’s architecture? Will it scale
> > >>> and
> > >>>>> keep
> > >>>>>>>> Flink flexible for the future, or will the feature restrict
> > >>> Flink
> > >>>> in
> > >>>>>> the
> > >>>>>>>> future? - Is the feature a significant new addition (rather
> > >>> than an
> > >>>>>>>> improvement to an existing part)? If yes, will the Flink
> > >>> community
> > >>>>>> commit
> > >>>>>>>> to maintaining this feature? - Does the feature produce added
> > >>> value
> > >>>>> for
> > >>>>>>>> Flink users or developers? Or does it introduce risk of
> > >>> regression
> > >>>>>>> without
> > >>>>>>>> adding relevant user or developer benefit?All of these questions
> > >>>>> should
> > >>>>>>> be
> > >>>>>>>> answerable from the description/discussion in JIRA and Pull
> > >>>> Request,
> > >>>>>>>> without looking at the code.(2) Does the contribution need
> > >>>> attention
> > >>>>>> from
> > >>>>>>>> some specific committers and is there time commitment from these
> > >>>>>>>> committers?Some changes require attention and approval from
> > >>>> specific
> > >>>>>>>> committers. For example, changes in parts that are either very
> > >>>>>>> performance
> > >>>>>>>> sensitive, or have a critical impact on distributed coordination
> > >>>> and
> > >>>>>>> fault
> > >>>>>>>> tolerance need input by a committer that is deeply familiar with
> > >>>> the
> > >>>>>>>> component.As a rule of thumb, this is the case when the Pull
> > >>>> Request
> > >>>>>>>> description answers one of the questions in the template section
> > >>>>> “Does
> > >>>>>>> this
> > >>>>>>>> pull request potentially affect one of the following parts” with
> > >>>>>>> ‘yes’.This
> > >>>>>>>> question can be answered with - Does not need specific
> > >>> attention-
> > >>>>> Needs
> > >>>>>>>> specific attention for X (X can be for example checkpointing,
> > >>>>>> jobmanager,
> > >>>>>>>> etc.).- Has specific attention for X by @commiterA,
> > >>> @contributorBIf
> > >>>>> the
> > >>>>>>>> pull request needs specific attention, one of the tagged
> > >>>>>>>> committers/contributors should give the final approval.(3) Is
> > >>> the
> > >>>>>>>> contribution described well?Check whether the contribution is
> > >>>>>>> sufficiently
> > >>>>>>>> well described to support a good review. Trivial changes and
> > >>> fixes
> > >>>> do
> > >>>>>> not
> > >>>>>>>> need a long description. Any pull request that changes
> > >>>> functionality
> > >>>>> or
> > >>>>>>>> behavior needs to describe the big picture of these changes, so
> > >>>> that
> > >>>>>>>> reviews know what to look for (and don’t have to dig through the
> > >>>> code
> > >>>>>> to
> > >>>>>>>> hopefully understand what the change does).Changes that require
> > >>>>> longer
> > >>>>>>>> descriptions are ideally based on a prior design discussion in
> > >>> the
> > >>>>>>> mailing
> > >>>>>>>> list or in JIRA and can simply link to there or copy the
> > >>>> description
> > >>>>>> from
> > >>>>>>>> there.(4) Does the implementation follow the right overall
> > >>>>>>>> approach/architecture?Is this the best approach to implement the
> > >>>> fix
> > >>>>> or
> > >>>>>>>> feature, or are there other approaches that would be easier,
> > >>> more
> > >>>>>> robust,
> > >>>>>>>> or more maintainable?This question should be answerable from the
> > >>>> Pull
> > >>>>>>>> Request description (or the linked JIRA) as much as possible.We
> > >>>>>> recommend
> > >>>>>>>> to check this before diving into the details of commenting on
> > >>>>>> individual
> > >>>>>>>> parts of the change.(5) Is the overall code quality good,
> > >>> meeting
> > >>>>>>> standard
> > >>>>>>>> we want to maintain in Flink?This is the detailed code review of
> > >>>> the
> > >>>>>>> actual
> > >>>>>>>> changes, covering: - Are the changes doing what is described in
> > >>> the
> > >>>>>>> design
> > >>>>>>>> document or PR description?- Does the code follow the right
> > >>>> software
> > >>>>>>>> engineering practices? It the code correct, robust,
> > >>> maintainable,
> > >>>>>>>> testable?- Are the change performance aware, when changing a
> > >>>>>> performance
> > >>>>>>>> sensitive part?- Are the changes sufficiently covered by tests?-
> > >>>> Are
> > >>>>>> the
> > >>>>>>>> tests executing fast?- Does the code format follow Flink’s
> > >>>> checkstyle
> > >>>>>>>> pattern?- Does the code avoid to introduce additional compiler
> > >>>>>>>> warnings?Some code style guidelines can be found in the [Flink
> > >>> Code
> > >>>>>> Style
> > >>>>>>>> Page](https://flink.apache.org/contribute-code.html#code-style
> > >>>>>>>> <https://flink.apache.org/contribute-code.html#code-style>)Pull
> > >>>>>> Request
> > >>>>>>>> Review TemplateAdd the following checklist to the pull request
> > >>>>> review,
> > >>>>>>>> checking the boxes as the questions are answered:  - [ ]
> > >>> Consensus
> > >>>>> that
> > >>>>>>> the
> > >>>>>>>> contribution should go into to Flink  - [ ] Does not need
> > >>> specific
> > >>>>>>>> attention | Needs specific attention for X | Has attention for X
> > >>>> by Y
> > >>>>>> -
> > >>>>>>> [
> > >>>>>>>> ] Contribution description  - [ ] Architectural approach  - [ ]
> > >>>>> Overall
> > >>>>>>>> code quality*
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

tison
Hi,

I would prefer the integrated idea and could help to concrete it.

Posting a review template once a pull request created. This is regarded as
the formal process reviews to follow. Posting a checklist in form and make
it editable for maintainer.

[ ] Reach consensus
[ ] Implementation architectural
[ ] Code quality admired
( can with more check item and description if needed )

When the process go ahead, maintainer, i.e., committers SHOULD update the
checklist, and SUGGESTED to add a label to mark the stage. Ideally when
stage changed, maintainer/committer add a corresponding comment to explain
it. Of course when things are obvious, some works could be omit.

Best,
tison.


Fabian Hueske <[hidden email]> 于2018年9月19日周三 下午8:14写道:

> Hi,
>
> I'd like to suggest that we keep this thread focused on discussing
> Stephan's proposal, i.e., introducing a structured PR review process.
> Tison and Piotr raised some good points related to PR reviews that are
> definitely worth discussing, but I think we should do that on different
> threads and move forward with the original discussion.
>
> Stephan's proposal has received only positive feedback until now.
> So, I think we should go ahead and adopt the process.
>
> IMO, there are two things to be done here:
>
> * Add the proposed process to the website (trivial)
> * Implement the process in practice which is still an open question to me.
>
> Two methods have been proposed to implement the process and I see following
> dis/advantages:
>
> 1. Automatically posting the review checklist as the first comment to a PR
> and track the progress by ticking off boxes in the checklist.
>
>   + First time contributors / reviewers learn about the process from the
> comment, which reduces the chance of detailed reviews without consensus on
> the motivation and approach.
>   - Comments are not visible when looking at the PR list and cannot be used
> for filtering.
>   - Needs some kind of service that automatically comments on PRs. Service
> does not need special permissions as every GH user can comment.
>
> 2. Tracking the review status of a PR with labels.
>
>   + Labels are visible on the PR overview and can be used to filter PRs.
>   - The review process is not spelled out. Contributors and reviewers have
> to learn about the process somewhere else (link could be added to PR
> template).
>   - If we want to tick-off labels, we needs some kind of service to
> automatically assign labels. The service needs committer permissions or be
> setup by ASF Infra.
>   - We need to check if assignment/removal of labels is mirrored to a
> mailing list which is important in the ASF to track decision. This
> shouldn't be hard to figure out, but if labels are not tracked, they cannot
> be the sole solution.
>
> We can of course also do both.
> Have the review checklist posted and tracked as a comment and ask reviewers
> to add the right label when ticking a box off.
>
> What do you think?
>
> Best,
> Fabian
>
> 2018-09-19 11:24 GMT+02:00 陈梓立 <[hidden email]>:
>
> > Hi Piotr,
> >
> > I strongly agree with your idea. Since we work on a huge project, diff in
> > the PR becomes hard to review if there is too much noise.
> >
> > Just to clarify the process. Do you mean that a pull request should go
> into
> > the way below?
> >
> > Separated commits during the implement, mainly distinguish feature/bug
> fix
> > with clean up/rework.
> >
> > [FLINK-XXX] [module] future/bug fix...
> > [FLINK-XXX] [module] more on future/bug fix...
> > [hotfix] clean up/rework...
> >
> > and so on.
> >
> > And finally, when get merged, put all stuff into one commit and comments
> > close #PRID
> > so coming ones could see the detail by jump to #PRID.
> >
> > One thing to trade off is that if we merge by one commit, we cannot
> revert
> > part of it automated; if we merge by PR commits(one by one), the commit
> log
> > might mess.
> >
> > Best,
> > tison.
> >
> >
> > Piotr Nowojski <[hidden email]> 于2018年9月19日周三 下午5:10写道:
> >
> > > Hi,
> > >
> > > I would like to rise one more issue. Often contributions are very heavy
> > > and difficult to review. They have one big commit that changes multiple
> > > things which is difficult to review. Instead I would be in favour of
> > > implementing a rule, that a single commit should never do more then one
> > > thing. For example never mixing refactoring/renames/clean ups/code
> > > deduplications with functional changes. If implementing something
> > requires
> > > two independent changes in two separate components, those also should
> be
> > > two independent commits. In other words, if there are two changed lines
> > in
> > > a single commit, they should interact somehow together and strictly
> > depend
> > > on one another. This has couple of important advantages:
> > >
> > > 1. Obviously faster reviews. Especially if a reviewer do not have to
> find
> > > 2 lines bug fix among 200 lines of renames/refactoring.
> > > 2. Provides good “cut out” points for reviewer. For example he can
> easily
> > > interrupt reviewing in the middle and continue later or even merge PR
> in
> > > stages.
> > > 3. Better reference “why something was done this way not the other” for
> > > the future. This is the same argument as first point, however with
> > benefit
> > > not during reviewing, but when after merging someone is trying to
> > > understand the code.
> > > 4. Commit message becomes much better place to write down reasons why
> > > something was done and what are the effects (not that this should
> replace
> > > comments/documentation, only to complement it).
> > > 5. In case of need to revert/drop some part of the contribution, we are
> > > not loosing all of it. If we have to revert some small feature, it
> would
> > be
> > > easier to keep refactoring and clean ups.
> > >
> > >
> > > Some examples of PRs that were more or less following this rule:
> > > https://github.com/apache/flink/pull/6692/commits
> > > https://github.com/apache/flink/pull/5423/commits <
> > > https://github.com/apache/flink/pull/5423/commits> (a bit extreme)
> > >
> > > If someone is not convinced I encourage to open those PRs and browse
> > > through couple of first commits (which are refactoring/clean up
> commits)
> > > one by one (GitHub has next/prev commit button). Then imagine if they
> > were
> > > squashed with some functional/performance improvement changes.
> > >
> > > Piotrek
> > >
> > > > On 18 Sep 2018, at 17:12, 陈梓立 <[hidden email]> wrote:
> > > >
> > > > Put some good cases here might be helpful.
> > > >
> > > > See how a contribution of runtime module be proposed, discussed,
> > > > implemented and merged from
> https://github.com/apache/flink/pull/5931
> > > to
> > > > https://github.com/apache/flink/pull/6132.
> > > >
> > > > 1. #5931 fix a bug, but remains points could be improved. Here
> > sihuazhou
> > > > and shuai-xu share their considerations and require review(of the
> > > proposal)
> > > > by Stephan, Till and Gary, our committers.
> > > > 2. After discussion, all people involved reach a consensus. So the
> rest
> > > > work is to implement it.
> > > > 3. sihuazhou gives out an implementation #6132, Till reviews it and
> > find
> > > it
> > > > is somewhat out of the "architectural" aspect, so suggests
> > > > implementation-level changes.
> > > > 4. Addressing those implementation-level comments, the PR gets
> merged.
> > > >
> > > > I think this is quite a good example how we think our review process
> > > should
> > > > go.
> > > >
> > > > Best,
> > > > tison.
> > > >
> > > >
> > > > 陈梓立 <[hidden email]> 于2018年9月18日周二 下午10:53写道:
> > > >
> > > >> Maybe a little rearrange to the process would help.
> > > >>
> > > >> (1). Does the contributor describe itself well?
> > > >>  (1.1) By whom this contribution should be given attention. This
> often
> > > >> shows by its title, "[FLINK-XXX] [module]", the module part infer.
> > > >>  (1.2) What the purpose of this contribution is. Done by the PR
> > > template.
> > > >> Even on JIRA an issue should cover these points.
> > > >>
> > > >> (2). Is there consensus on the contribution?
> > > >> This follows (1), because we need to clear what the purpose of the
> > > >> contribution first. At this stage reviewers could cc to module
> > > maintainer
> > > >> as a supplement to (1.1). Also reviewers might ask the contributor
> to
> > > >> clarify his purpose to sharp(1.2)
> > > >>
> > > >> (3). Is the implement architectural and fit code style?
> > > >> This follows (2). And only after a consensus we talk about concrete
> > > >> implement, which prevent spend time and put effort in vain.
> > > >>
> > > >> In addition, ideally a "+1" comment or approval means the purpose of
> > > >> contribution is supported by the reviewer and implement(if there is)
> > > >> quality is fine, so the reviewer vote for a consensus.
> > > >>
> > > >> Best,
> > > >> tison.
> > > >>
> > > >>
> > > >> Stephan Ewen <[hidden email]> 于2018年9月18日周二 下午6:44写道:
> > > >>
> > > >>> On the template discussion, some thoughts
> > > >>>
> > > >>> *PR Template*
> > > >>>
> > > >>> I think the PR template went well. We can rethink the "checklist"
> at
> > > the
> > > >>> bottom, but all other parts turned out helpful in my opinion.
> > > >>>
> > > >>> With the amount of contributions, it helps to ask the contributor
> to
> > > take
> > > >>> a
> > > >>> little more work in order for the reviewer to be more efficient.
> > > >>> I would suggest to keep that mindset: Whenever we find a way that
> the
> > > >>> contributor can prepare stuff in such a way that reviews become
> > > >>> more efficient, we should do that. In my experience, most
> > contributors
> > > are
> > > >>> willing to put in some extra minutes if it helps that their
> > > >>> PR gets merged faster.
> > > >>>
> > > >>> *Review Template*
> > > >>>
> > > >>> I think it would be helpful to have this checklist. It does not
> > matter
> > > in
> > > >>> which form, be that as a text template, be that as labels.
> > > >>>
> > > >>> The most important thing is to make explicit which questions have
> > been
> > > >>> answered in the review.
> > > >>> Currently there is a lot of "+1" on pull requests which means "code
> > > >>> quality
> > > >>> is fine", but all other questions are unanswered.
> > > >>> The contributors then rightfully wonder why this does not get
> merged.
> > > >>>
> > > >>>
> > > >>>
> > > >>> On Tue, Sep 18, 2018 at 7:26 AM, 陈梓立 <[hidden email]> wrote:
> > > >>>
> > > >>>> Hi all interested,
> > > >>>>
> > > >>>> Within the document there is a heated discussion about how the PR
> > > >>>> template/review template should be.
> > > >>>>
> > > >>>> Here share my opinion:
> > > >>>>
> > > >>>> 1. For the review template, actually we don't need comment a
> review
> > > >>>> template at all. GitHub has a tag system and only committer could
> > add
> > > >>> tags,
> > > >>>> which we can make use of it. That is, tagging this PR is
> > > >>>> waiting-for-proposal-approved, waiting-for-code-review,
> > > >>>> waiting-for-benchmark or block-by-author and so on. Asfbot could
> > pick
> > > >>>> GitHub tag state to the corresponding JIRA and we always regard
> JIRA
> > > as
> > > >>> the
> > > >>>> main discussion borad.
> > > >>>>
> > > >>>> 2. For the PR template, the greeting message is redundant. Just
> > > >>> emphasize a
> > > >>>> JIRA associated is important and how to format the title is
> enough.
> > > >>>> Besides, the "Does this pull request potentially affect one of the
> > > >>>> following parts" part and "Documentation" should be coved from
> "What
> > > is
> > > >>> the
> > > >>>> purpose of the change" and "Brief change log". These two parts,
> > users
> > > >>>> always answer no and would be aware if they really make changes on
> > it.
> > > >>> As
> > > >>>> example, even pull request requires document, its owner might no
> add
> > > it
> > > >>> at
> > > >>>> first. The PR template is a guide but not which one have to learn.
> > > >>>>
> > > >>>> To sum up, (1) take advantage of GitHub's tag system to tag review
> > > >>> progress
> > > >>>> (2) make the template more concise to avoid burden mature
> > contributors
> > > >>> and
> > > >>>> force new comer to learn too much.
> > > >>>>
> > > >>>> Best,
> > > >>>> tison.
> > > >>>>
> > > >>>>
> > > >>>> Rong Rong <[hidden email]> 于2018年9月18日周二 上午7:05写道:
> > > >>>>
> > > >>>>> Thanks for putting the review contribution doc together, Stephan!
> > > This
> > > >>>> will
> > > >>>>> definitely help the community to make the review process better.
> > > >>>>>
> > > >>>>> From my experience this will benefit on both contributors and
> > > >>> reviewers
> > > >>>>> side! Thus +1 for putting into practice as well.
> > > >>>>>
> > > >>>>> --
> > > >>>>> Rong
> > > >>>>>
> > > >>>>> On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]>
> > > >>> wrote:
> > > >>>>>
> > > >>>>>> Hi!
> > > >>>>>>
> > > >>>>>> Thanks you for the encouraging feedback so far.
> > > >>>>>>
> > > >>>>>> The overall goal is definitely to make the contribution process
> > > >>> better
> > > >>>>> and
> > > >>>>>> get fewer pull requests that are disregarded.
> > > >>>>>>
> > > >>>>>> There are various reasons for the disregarded pull requests, one
> > > >>> being
> > > >>>>> that
> > > >>>>>> fewer committers really participate in reviews beyond
> > > >>>>>> the component they are currently very involved with. This is a
> > > >>> separate
> > > >>>>>> issue and I am thinking on how to encourage more
> > > >>>>>> activity there.
> > > >>>>>>
> > > >>>>>> The other reason I was lack of structure and lack of decision
> > > >>> making,
> > > >>>>> which
> > > >>>>>> is what I am first trying to fix here.
> > > >>>>>> A follow-up to this will definitely be to improve the
> contribution
> > > >>>> guide
> > > >>>>> as
> > > >>>>>> well.
> > > >>>>>>
> > > >>>>>> Best,
> > > >>>>>> Stephan
> > > >>>>>>
> > > >>>>>>
> > > >>>>>> On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
> > > >>>>>> [hidden email]> wrote:
> > > >>>>>>
> > > >>>>>>> From my personal experience as a contributor for three years, I
> > > >>> feel
> > > >>>>>>> better experience in contirbuting or reviewing than before,
> > > >>> although
> > > >>>> we
> > > >>>>>>> still have some points for further progress.
> > > >>>>>>>
> > > >>>>>>> I reviewed the proposal doc, and it gives very constructive and
> > > >>>>>> meaningful
> > > >>>>>>> guides which could help both contributor and reviewer. I agree
> > > >>> with
> > > >>>> the
> > > >>>>>>> bove suggestions and wish they can be praticed well!
> > > >>>>>>>
> > > >>>>>>> Best,
> > > >>>>>>> Zhijiang
> > > >>>>>>> ------------------------------------------------------------
> > ------
> > > >>>>>>> 发件人:Till Rohrmann <[hidden email]>
> > > >>>>>>> 发送时间:2018年9月17日(星期一) 16:27
> > > >>>>>>> 收件人:dev <[hidden email]>
> > > >>>>>>> 主 题:Re: [PROPOSAL] [community] A more structured approach to
> > > >>> reviews
> > > >>>>> and
> > > >>>>>>> contributions
> > > >>>>>>>
> > > >>>>>>> Thanks for writing this up Stephan. I like the steps and hope
> > > >>> that it
> > > >>>>>> will
> > > >>>>>>> help the community to make the review process better. Thus, +1
> > for
> > > >>>>>> putting
> > > >>>>>>> your proposal to practice.
> > > >>>>>>>
> > > >>>>>>> Cheers,
> > > >>>>>>> Till
> > > >>>>>>>
> > > >>>>>>> On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <
> [hidden email]>
> > > >>>>> wrote:
> > > >>>>>>>
> > > >>>>>>>> Hi Flink community members!
> > > >>>>>>>>
> > > >>>>>>>> As many of you will have noticed, the Flink project activity
> has
> > > >>>> gone
> > > >>>>>> up
> > > >>>>>>>> again quite a bit.
> > > >>>>>>>> There are many more contributions, which is an absolutely
> great
> > > >>>> thing
> > > >>>>>> to
> > > >>>>>>>> have :-)
> > > >>>>>>>>
> > > >>>>>>>> However, we see a continuously growing backlog of pull
> requests
> > > >>> and
> > > >>>>>> JIRA
> > > >>>>>>>> issues.
> > > >>>>>>>> To make sure the community will be able to handle the
> increased
> > > >>>>>> volume, I
> > > >>>>>>>> think we need to revisit some
> > > >>>>>>>> approaches and processes. I believe there are a few
> > > >>> opportunities
> > > >>>> to
> > > >>>>>>>> structure things a bit better, which
> > > >>>>>>>> should help to scale the development.
> > > >>>>>>>>
> > > >>>>>>>> The first thing I would like to bring up are *Pull Request
> > > >>>> Reviews*.
> > > >>>>>> Even
> > > >>>>>>>> though more community members being
> > > >>>>>>>> active in reviews (which is a really great thing!) the Pull
> > > >>> Request
> > > >>>>>>> backlog
> > > >>>>>>>> is increasing quite a bit.
> > > >>>>>>>>
> > > >>>>>>>> Why are pull requests still not merged faster? Looking at the
> > > >>>>> reviews,
> > > >>>>>>> one
> > > >>>>>>>> thing I noticed is that most reviews deal
> > > >>>>>>>> immediately with detailed code issues, and leave out most of
> the
> > > >>>> core
> > > >>>>>>>> questions that need to be answered
> > > >>>>>>>> before a Pull Request can be merged, like "is this a desired
> > > >>>>> feature?"
> > > >>>>>> or
> > > >>>>>>>> "does this align well with other developments?".
> > > >>>>>>>> I think that we even make things slightly worse that way: From
> > > >>> my
> > > >>>>>>> personal
> > > >>>>>>>> experience, I have often thought "oh, this
> > > >>>>>>>> PR has a review already" and rather looked at another PR, only
> > > >>> to
> > > >>>>> find
> > > >>>>>>>> later that the first review did never decide whether
> > > >>>>>>>> this PR is actually a good fit for Flink.
> > > >>>>>>>>
> > > >>>>>>>> There has never been a proper documentation of how to answer
> > > >>> these
> > > >>>>>>>> questions, what to evaluate in reviews,
> > > >>>>>>>> guidelines for how to evaluate pull requests, other than code
> > > >>>>> quality.
> > > >>>>>> I
> > > >>>>>>>> suspect that this is why so many reviewers
> > > >>>>>>>> do not address the "is this a good contribution" questions,
> > > >>> making
> > > >>>>> pull
> > > >>>>>>>> requests linger until another committers joins
> > > >>>>>>>> the review.
> > > >>>>>>>>
> > > >>>>>>>> Below is an idea for a guide *"How to Review Contributions"*.
> It
> > > >>>>>> outlines
> > > >>>>>>>> five core aspects to be checked in every
> > > >>>>>>>> pull request, and suggests a priority for clarifying those.
> The
> > > >>>> idea
> > > >>>>> is
> > > >>>>>>>> that this helps us to better structure reviews, and
> > > >>>>>>>> to make each reviewer aware what we look for in a review and
> > > >>> where
> > > >>>> to
> > > >>>>>>> best
> > > >>>>>>>> bring in their help.
> > > >>>>>>>>
> > > >>>>>>>> Looking forward to comments!
> > > >>>>>>>>
> > > >>>>>>>> Best,
> > > >>>>>>>> Stephan
> > > >>>>>>>>
> > > >>>>>>>> ====================================
> > > >>>>>>>>
> > > >>>>>>>> The draft is in this Google Doc. Please add small textual
> > > >>> comments
> > > >>>> to
> > > >>>>>> the
> > > >>>>>>>> doc, and bigger principle discussions as replies to this mail.
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>> https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
> > > >>>>>>> RbocGlGKCYnvJd9lVhk/edit?usp=sharing
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>> *How to Review Contributions------------------------------This
> > > >>>> guide
> > > >>>>> is
> > > >>>>>>> for
> > > >>>>>>>> all committers and contributors that want to help with
> reviewing
> > > >>>>>>>> contributions. Thank you for your effort - good reviews are
> one
> > > >>> the
> > > >>>>>> most
> > > >>>>>>>> important and crucial parts of an open source project. This
> > > >>> guide
> > > >>>>>> should
> > > >>>>>>>> help the community to make reviews such that: - Contributors
> > > >>> have a
> > > >>>>>> good
> > > >>>>>>>> contribution experience- Reviews are structured and check all
> > > >>>>> important
> > > >>>>>>>> aspects of a contribution- Make sure we keep a high code
> > > >>> quality in
> > > >>>>>>> Flink-
> > > >>>>>>>> We avoid situations where contributors and reviewers spend a
> > > >>> lot of
> > > >>>>>> time
> > > >>>>>>> to
> > > >>>>>>>> refine a contribution that gets rejected laterReview
> > > >>> ChecklistEvery
> > > >>>>>>> review
> > > >>>>>>>> needs to check the following five aspects. We encourage to
> check
> > > >>>>> these
> > > >>>>>>>> aspects in order, to avoid spending time on detailed code
> > > >>> quality
> > > >>>>>> reviews
> > > >>>>>>>> when there is not yet consensus that a feature or change
> should
> > > >>> be
> > > >>>>>>> actually
> > > >>>>>>>> be added.(1) Is there consensus whether the change of feature
> > > >>>> should
> > > >>>>> go
> > > >>>>>>>> into to Flink?For bug fixes, this needs to be checked only in
> > > >>> case
> > > >>>> it
> > > >>>>>>>> requires bigger changes or might break existing programs and
> > > >>>>>>>> setups.Ideally, this question is already answered from a JIRA
> > > >>> issue
> > > >>>>> or
> > > >>>>>>> the
> > > >>>>>>>> dev-list discussion, except in cases of bug fixes and small
> > > >>>>> lightweight
> > > >>>>>>>> additions/extensions. In that case, this question can be
> > > >>>> immediately
> > > >>>>>>> marked
> > > >>>>>>>> as resolved. For pull requests that are created without prior
> > > >>>>>> consensus,
> > > >>>>>>>> this question needs to be answered as part of the review.The
> > > >>>> decision
> > > >>>>>>>> whether the change should go into Flink needs to take the
> > > >>> following
> > > >>>>>>> aspects
> > > >>>>>>>> into consideration: - Does the contribution alter the behavior
> > > >>> of
> > > >>>>>>> features
> > > >>>>>>>> or components in a way that it may break previous users’
> > > >>> programs
> > > >>>> and
> > > >>>>>>>> setups? If yes, there needs to be a discussion and agreement
> > > >>> that
> > > >>>>> this
> > > >>>>>>>> change is desirable. - Does the contribution conceptually fit
> > > >>> well
> > > >>>>> into
> > > >>>>>>>> Flink? Is it too much of special case such that it makes
> things
> > > >>>> more
> > > >>>>>>>> complicated for the common case, or bloats the abstractions /
> > > >>>> APIs? -
> > > >>>>>>> Does
> > > >>>>>>>> the feature fit well into Flink’s architecture? Will it scale
> > > >>> and
> > > >>>>> keep
> > > >>>>>>>> Flink flexible for the future, or will the feature restrict
> > > >>> Flink
> > > >>>> in
> > > >>>>>> the
> > > >>>>>>>> future? - Is the feature a significant new addition (rather
> > > >>> than an
> > > >>>>>>>> improvement to an existing part)? If yes, will the Flink
> > > >>> community
> > > >>>>>> commit
> > > >>>>>>>> to maintaining this feature? - Does the feature produce added
> > > >>> value
> > > >>>>> for
> > > >>>>>>>> Flink users or developers? Or does it introduce risk of
> > > >>> regression
> > > >>>>>>> without
> > > >>>>>>>> adding relevant user or developer benefit?All of these
> questions
> > > >>>>> should
> > > >>>>>>> be
> > > >>>>>>>> answerable from the description/discussion in JIRA and Pull
> > > >>>> Request,
> > > >>>>>>>> without looking at the code.(2) Does the contribution need
> > > >>>> attention
> > > >>>>>> from
> > > >>>>>>>> some specific committers and is there time commitment from
> these
> > > >>>>>>>> committers?Some changes require attention and approval from
> > > >>>> specific
> > > >>>>>>>> committers. For example, changes in parts that are either very
> > > >>>>>>> performance
> > > >>>>>>>> sensitive, or have a critical impact on distributed
> coordination
> > > >>>> and
> > > >>>>>>> fault
> > > >>>>>>>> tolerance need input by a committer that is deeply familiar
> with
> > > >>>> the
> > > >>>>>>>> component.As a rule of thumb, this is the case when the Pull
> > > >>>> Request
> > > >>>>>>>> description answers one of the questions in the template
> section
> > > >>>>> “Does
> > > >>>>>>> this
> > > >>>>>>>> pull request potentially affect one of the following parts”
> with
> > > >>>>>>> ‘yes’.This
> > > >>>>>>>> question can be answered with - Does not need specific
> > > >>> attention-
> > > >>>>> Needs
> > > >>>>>>>> specific attention for X (X can be for example checkpointing,
> > > >>>>>> jobmanager,
> > > >>>>>>>> etc.).- Has specific attention for X by @commiterA,
> > > >>> @contributorBIf
> > > >>>>> the
> > > >>>>>>>> pull request needs specific attention, one of the tagged
> > > >>>>>>>> committers/contributors should give the final approval.(3) Is
> > > >>> the
> > > >>>>>>>> contribution described well?Check whether the contribution is
> > > >>>>>>> sufficiently
> > > >>>>>>>> well described to support a good review. Trivial changes and
> > > >>> fixes
> > > >>>> do
> > > >>>>>> not
> > > >>>>>>>> need a long description. Any pull request that changes
> > > >>>> functionality
> > > >>>>> or
> > > >>>>>>>> behavior needs to describe the big picture of these changes,
> so
> > > >>>> that
> > > >>>>>>>> reviews know what to look for (and don’t have to dig through
> the
> > > >>>> code
> > > >>>>>> to
> > > >>>>>>>> hopefully understand what the change does).Changes that
> require
> > > >>>>> longer
> > > >>>>>>>> descriptions are ideally based on a prior design discussion in
> > > >>> the
> > > >>>>>>> mailing
> > > >>>>>>>> list or in JIRA and can simply link to there or copy the
> > > >>>> description
> > > >>>>>> from
> > > >>>>>>>> there.(4) Does the implementation follow the right overall
> > > >>>>>>>> approach/architecture?Is this the best approach to implement
> the
> > > >>>> fix
> > > >>>>> or
> > > >>>>>>>> feature, or are there other approaches that would be easier,
> > > >>> more
> > > >>>>>> robust,
> > > >>>>>>>> or more maintainable?This question should be answerable from
> the
> > > >>>> Pull
> > > >>>>>>>> Request description (or the linked JIRA) as much as
> possible.We
> > > >>>>>> recommend
> > > >>>>>>>> to check this before diving into the details of commenting on
> > > >>>>>> individual
> > > >>>>>>>> parts of the change.(5) Is the overall code quality good,
> > > >>> meeting
> > > >>>>>>> standard
> > > >>>>>>>> we want to maintain in Flink?This is the detailed code review
> of
> > > >>>> the
> > > >>>>>>> actual
> > > >>>>>>>> changes, covering: - Are the changes doing what is described
> in
> > > >>> the
> > > >>>>>>> design
> > > >>>>>>>> document or PR description?- Does the code follow the right
> > > >>>> software
> > > >>>>>>>> engineering practices? It the code correct, robust,
> > > >>> maintainable,
> > > >>>>>>>> testable?- Are the change performance aware, when changing a
> > > >>>>>> performance
> > > >>>>>>>> sensitive part?- Are the changes sufficiently covered by
> tests?-
> > > >>>> Are
> > > >>>>>> the
> > > >>>>>>>> tests executing fast?- Does the code format follow Flink’s
> > > >>>> checkstyle
> > > >>>>>>>> pattern?- Does the code avoid to introduce additional compiler
> > > >>>>>>>> warnings?Some code style guidelines can be found in the [Flink
> > > >>> Code
> > > >>>>>> Style
> > > >>>>>>>> Page](
> https://flink.apache.org/contribute-code.html#code-style
> > > >>>>>>>> <https://flink.apache.org/contribute-code.html#code-style
> >)Pull
> > > >>>>>> Request
> > > >>>>>>>> Review TemplateAdd the following checklist to the pull request
> > > >>>>> review,
> > > >>>>>>>> checking the boxes as the questions are answered:  - [ ]
> > > >>> Consensus
> > > >>>>> that
> > > >>>>>>> the
> > > >>>>>>>> contribution should go into to Flink  - [ ] Does not need
> > > >>> specific
> > > >>>>>>>> attention | Needs specific attention for X | Has attention
> for X
> > > >>>> by Y
> > > >>>>>> -
> > > >>>>>>> [
> > > >>>>>>>> ] Contribution description  - [ ] Architectural approach  - [
> ]
> > > >>>>> Overall
> > > >>>>>>>> code quality*
> > > >>>>>>>>
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>
> > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

Thomas Weise
In reply to this post by tison
Follow-up regarding the PR template that pops up when opening a PR:

I think what we have now is a fairly big blob of text that jumps up a bit
unexpectedly for a first time contributor and is also cumbersome to deal
with in the small PR description window. Perhaps we can improve it a bit:

* Instead of putting all that text into the description, add it to
website/wiki and just have a pointer in the PR, asking the contributor to
review the guidelines before opening a PR.
* If the questions further down can be made relevant to the context of the
contribution, that would probably help both the contributor and the
reviewer. For example, the questions would be different for a documentation
change, connector change or work deep in core. Not sure if that can be
automated, but if moved to a separate page, it could be structured better.

Thanks,
Thomas






On Tue, Sep 18, 2018 at 8:13 AM 陈梓立 <[hidden email]> wrote:

> Put some good cases here might be helpful.
>
> See how a contribution of runtime module be proposed, discussed,
> implemented and merged from  https://github.com/apache/flink/pull/5931 to
> https://github.com/apache/flink/pull/6132.
>
> 1. #5931 fix a bug, but remains points could be improved. Here sihuazhou
> and shuai-xu share their considerations and require review(of the proposal)
> by Stephan, Till and Gary, our committers.
> 2. After discussion, all people involved reach a consensus. So the rest
> work is to implement it.
> 3. sihuazhou gives out an implementation #6132, Till reviews it and find it
> is somewhat out of the "architectural" aspect, so suggests
> implementation-level changes.
> 4. Addressing those implementation-level comments, the PR gets merged.
>
> I think this is quite a good example how we think our review process should
> go.
>
> Best,
> tison.
>
>
> 陈梓立 <[hidden email]> 于2018年9月18日周二 下午10:53写道:
>
> > Maybe a little rearrange to the process would help.
> >
> > (1). Does the contributor describe itself well?
> >   (1.1) By whom this contribution should be given attention. This often
> > shows by its title, "[FLINK-XXX] [module]", the module part infer.
> >   (1.2) What the purpose of this contribution is. Done by the PR
> template.
> > Even on JIRA an issue should cover these points.
> >
> > (2). Is there consensus on the contribution?
> > This follows (1), because we need to clear what the purpose of the
> > contribution first. At this stage reviewers could cc to module maintainer
> > as a supplement to (1.1). Also reviewers might ask the contributor to
> > clarify his purpose to sharp(1.2)
> >
> > (3). Is the implement architectural and fit code style?
> > This follows (2). And only after a consensus we talk about concrete
> > implement, which prevent spend time and put effort in vain.
> >
> > In addition, ideally a "+1" comment or approval means the purpose of
> > contribution is supported by the reviewer and implement(if there is)
> > quality is fine, so the reviewer vote for a consensus.
> >
> > Best,
> > tison.
> >
> >
> > Stephan Ewen <[hidden email]> 于2018年9月18日周二 下午6:44写道:
> >
> >> On the template discussion, some thoughts
> >>
> >> *PR Template*
> >>
> >> I think the PR template went well. We can rethink the "checklist" at the
> >> bottom, but all other parts turned out helpful in my opinion.
> >>
> >> With the amount of contributions, it helps to ask the contributor to
> take
> >> a
> >> little more work in order for the reviewer to be more efficient.
> >> I would suggest to keep that mindset: Whenever we find a way that the
> >> contributor can prepare stuff in such a way that reviews become
> >> more efficient, we should do that. In my experience, most contributors
> are
> >> willing to put in some extra minutes if it helps that their
> >> PR gets merged faster.
> >>
> >> *Review Template*
> >>
> >> I think it would be helpful to have this checklist. It does not matter
> in
> >> which form, be that as a text template, be that as labels.
> >>
> >> The most important thing is to make explicit which questions have been
> >> answered in the review.
> >> Currently there is a lot of "+1" on pull requests which means "code
> >> quality
> >> is fine", but all other questions are unanswered.
> >> The contributors then rightfully wonder why this does not get merged.
> >>
> >>
> >>
> >> On Tue, Sep 18, 2018 at 7:26 AM, 陈梓立 <[hidden email]> wrote:
> >>
> >> > Hi all interested,
> >> >
> >> > Within the document there is a heated discussion about how the PR
> >> > template/review template should be.
> >> >
> >> > Here share my opinion:
> >> >
> >> > 1. For the review template, actually we don't need comment a review
> >> > template at all. GitHub has a tag system and only committer could add
> >> tags,
> >> > which we can make use of it. That is, tagging this PR is
> >> > waiting-for-proposal-approved, waiting-for-code-review,
> >> > waiting-for-benchmark or block-by-author and so on. Asfbot could pick
> >> > GitHub tag state to the corresponding JIRA and we always regard JIRA
> as
> >> the
> >> > main discussion borad.
> >> >
> >> > 2. For the PR template, the greeting message is redundant. Just
> >> emphasize a
> >> > JIRA associated is important and how to format the title is enough.
> >> > Besides, the "Does this pull request potentially affect one of the
> >> > following parts" part and "Documentation" should be coved from "What
> is
> >> the
> >> > purpose of the change" and "Brief change log". These two parts, users
> >> > always answer no and would be aware if they really make changes on it.
> >> As
> >> > example, even pull request requires document, its owner might no add
> it
> >> at
> >> > first. The PR template is a guide but not which one have to learn.
> >> >
> >> > To sum up, (1) take advantage of GitHub's tag system to tag review
> >> progress
> >> > (2) make the template more concise to avoid burden mature contributors
> >> and
> >> > force new comer to learn too much.
> >> >
> >> > Best,
> >> > tison.
> >> >
> >> >
> >> > Rong Rong <[hidden email]> 于2018年9月18日周二 上午7:05写道:
> >> >
> >> > > Thanks for putting the review contribution doc together, Stephan!
> This
> >> > will
> >> > > definitely help the community to make the review process better.
> >> > >
> >> > > From my experience this will benefit on both contributors and
> >> reviewers
> >> > > side! Thus +1 for putting into practice as well.
> >> > >
> >> > > --
> >> > > Rong
> >> > >
> >> > > On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]>
> >> wrote:
> >> > >
> >> > > > Hi!
> >> > > >
> >> > > > Thanks you for the encouraging feedback so far.
> >> > > >
> >> > > > The overall goal is definitely to make the contribution process
> >> better
> >> > > and
> >> > > > get fewer pull requests that are disregarded.
> >> > > >
> >> > > > There are various reasons for the disregarded pull requests, one
> >> being
> >> > > that
> >> > > > fewer committers really participate in reviews beyond
> >> > > > the component they are currently very involved with. This is a
> >> separate
> >> > > > issue and I am thinking on how to encourage more
> >> > > > activity there.
> >> > > >
> >> > > > The other reason I was lack of structure and lack of decision
> >> making,
> >> > > which
> >> > > > is what I am first trying to fix here.
> >> > > > A follow-up to this will definitely be to improve the contribution
> >> > guide
> >> > > as
> >> > > > well.
> >> > > >
> >> > > > Best,
> >> > > > Stephan
> >> > > >
> >> > > >
> >> > > > On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
> >> > > > [hidden email]> wrote:
> >> > > >
> >> > > > > From my personal experience as a contributor for three years, I
> >> feel
> >> > > > > better experience in contirbuting or reviewing than before,
> >> although
> >> > we
> >> > > > > still have some points for further progress.
> >> > > > >
> >> > > > > I reviewed the proposal doc, and it gives very constructive and
> >> > > > meaningful
> >> > > > > guides which could help both contributor and reviewer. I agree
> >> with
> >> > the
> >> > > > > bove suggestions and wish they can be praticed well!
> >> > > > >
> >> > > > > Best,
> >> > > > > Zhijiang
> >> > > > >
> ------------------------------------------------------------------
> >> > > > > 发件人:Till Rohrmann <[hidden email]>
> >> > > > > 发送时间:2018年9月17日(星期一) 16:27
> >> > > > > 收件人:dev <[hidden email]>
> >> > > > > 主 题:Re: [PROPOSAL] [community] A more structured approach to
> >> reviews
> >> > > and
> >> > > > > contributions
> >> > > > >
> >> > > > > Thanks for writing this up Stephan. I like the steps and hope
> >> that it
> >> > > > will
> >> > > > > help the community to make the review process better. Thus, +1
> for
> >> > > > putting
> >> > > > > your proposal to practice.
> >> > > > >
> >> > > > > Cheers,
> >> > > > > Till
> >> > > > >
> >> > > > > On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <[hidden email]
> >
> >> > > wrote:
> >> > > > >
> >> > > > > > Hi Flink community members!
> >> > > > > >
> >> > > > > > As many of you will have noticed, the Flink project activity
> has
> >> > gone
> >> > > > up
> >> > > > > > again quite a bit.
> >> > > > > > There are many more contributions, which is an absolutely
> great
> >> > thing
> >> > > > to
> >> > > > > > have :-)
> >> > > > > >
> >> > > > > > However, we see a continuously growing backlog of pull
> requests
> >> and
> >> > > > JIRA
> >> > > > > > issues.
> >> > > > > > To make sure the community will be able to handle the
> increased
> >> > > > volume, I
> >> > > > > > think we need to revisit some
> >> > > > > > approaches and processes. I believe there are a few
> >> opportunities
> >> > to
> >> > > > > > structure things a bit better, which
> >> > > > > > should help to scale the development.
> >> > > > > >
> >> > > > > > The first thing I would like to bring up are *Pull Request
> >> > Reviews*.
> >> > > > Even
> >> > > > > > though more community members being
> >> > > > > > active in reviews (which is a really great thing!) the Pull
> >> Request
> >> > > > > backlog
> >> > > > > > is increasing quite a bit.
> >> > > > > >
> >> > > > > > Why are pull requests still not merged faster? Looking at the
> >> > > reviews,
> >> > > > > one
> >> > > > > > thing I noticed is that most reviews deal
> >> > > > > > immediately with detailed code issues, and leave out most of
> the
> >> > core
> >> > > > > > questions that need to be answered
> >> > > > > > before a Pull Request can be merged, like "is this a desired
> >> > > feature?"
> >> > > > or
> >> > > > > > "does this align well with other developments?".
> >> > > > > > I think that we even make things slightly worse that way: From
> >> my
> >> > > > > personal
> >> > > > > > experience, I have often thought "oh, this
> >> > > > > > PR has a review already" and rather looked at another PR, only
> >> to
> >> > > find
> >> > > > > > later that the first review did never decide whether
> >> > > > > > this PR is actually a good fit for Flink.
> >> > > > > >
> >> > > > > > There has never been a proper documentation of how to answer
> >> these
> >> > > > > > questions, what to evaluate in reviews,
> >> > > > > > guidelines for how to evaluate pull requests, other than code
> >> > > quality.
> >> > > > I
> >> > > > > > suspect that this is why so many reviewers
> >> > > > > > do not address the "is this a good contribution" questions,
> >> making
> >> > > pull
> >> > > > > > requests linger until another committers joins
> >> > > > > > the review.
> >> > > > > >
> >> > > > > > Below is an idea for a guide *"How to Review Contributions"*.
> It
> >> > > > outlines
> >> > > > > > five core aspects to be checked in every
> >> > > > > > pull request, and suggests a priority for clarifying those.
> The
> >> > idea
> >> > > is
> >> > > > > > that this helps us to better structure reviews, and
> >> > > > > > to make each reviewer aware what we look for in a review and
> >> where
> >> > to
> >> > > > > best
> >> > > > > > bring in their help.
> >> > > > > >
> >> > > > > > Looking forward to comments!
> >> > > > > >
> >> > > > > > Best,
> >> > > > > > Stephan
> >> > > > > >
> >> > > > > > ====================================
> >> > > > > >
> >> > > > > > The draft is in this Google Doc. Please add small textual
> >> comments
> >> > to
> >> > > > the
> >> > > > > > doc, and bigger principle discussions as replies to this mail.
> >> > > > > >
> >> > > > > >
> >> > > > > > https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
> >> > > > > RbocGlGKCYnvJd9lVhk/edit?usp=sharing
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > > *How to Review Contributions------------------------------This
> >> > guide
> >> > > is
> >> > > > > for
> >> > > > > > all committers and contributors that want to help with
> reviewing
> >> > > > > > contributions. Thank you for your effort - good reviews are
> one
> >> the
> >> > > > most
> >> > > > > > important and crucial parts of an open source project. This
> >> guide
> >> > > > should
> >> > > > > > help the community to make reviews such that: - Contributors
> >> have a
> >> > > > good
> >> > > > > > contribution experience- Reviews are structured and check all
> >> > > important
> >> > > > > > aspects of a contribution- Make sure we keep a high code
> >> quality in
> >> > > > > Flink-
> >> > > > > > We avoid situations where contributors and reviewers spend a
> >> lot of
> >> > > > time
> >> > > > > to
> >> > > > > > refine a contribution that gets rejected laterReview
> >> ChecklistEvery
> >> > > > > review
> >> > > > > > needs to check the following five aspects. We encourage to
> check
> >> > > these
> >> > > > > > aspects in order, to avoid spending time on detailed code
> >> quality
> >> > > > reviews
> >> > > > > > when there is not yet consensus that a feature or change
> should
> >> be
> >> > > > > actually
> >> > > > > > be added.(1) Is there consensus whether the change of feature
> >> > should
> >> > > go
> >> > > > > > into to Flink?For bug fixes, this needs to be checked only in
> >> case
> >> > it
> >> > > > > > requires bigger changes or might break existing programs and
> >> > > > > > setups.Ideally, this question is already answered from a JIRA
> >> issue
> >> > > or
> >> > > > > the
> >> > > > > > dev-list discussion, except in cases of bug fixes and small
> >> > > lightweight
> >> > > > > > additions/extensions. In that case, this question can be
> >> > immediately
> >> > > > > marked
> >> > > > > > as resolved. For pull requests that are created without prior
> >> > > > consensus,
> >> > > > > > this question needs to be answered as part of the review.The
> >> > decision
> >> > > > > > whether the change should go into Flink needs to take the
> >> following
> >> > > > > aspects
> >> > > > > > into consideration: - Does the contribution alter the behavior
> >> of
> >> > > > > features
> >> > > > > > or components in a way that it may break previous users’
> >> programs
> >> > and
> >> > > > > > setups? If yes, there needs to be a discussion and agreement
> >> that
> >> > > this
> >> > > > > > change is desirable. - Does the contribution conceptually fit
> >> well
> >> > > into
> >> > > > > > Flink? Is it too much of special case such that it makes
> things
> >> > more
> >> > > > > > complicated for the common case, or bloats the abstractions /
> >> > APIs? -
> >> > > > > Does
> >> > > > > > the feature fit well into Flink’s architecture? Will it scale
> >> and
> >> > > keep
> >> > > > > > Flink flexible for the future, or will the feature restrict
> >> Flink
> >> > in
> >> > > > the
> >> > > > > > future? - Is the feature a significant new addition (rather
> >> than an
> >> > > > > > improvement to an existing part)? If yes, will the Flink
> >> community
> >> > > > commit
> >> > > > > > to maintaining this feature? - Does the feature produce added
> >> value
> >> > > for
> >> > > > > > Flink users or developers? Or does it introduce risk of
> >> regression
> >> > > > > without
> >> > > > > > adding relevant user or developer benefit?All of these
> questions
> >> > > should
> >> > > > > be
> >> > > > > > answerable from the description/discussion in JIRA and Pull
> >> > Request,
> >> > > > > > without looking at the code.(2) Does the contribution need
> >> > attention
> >> > > > from
> >> > > > > > some specific committers and is there time commitment from
> these
> >> > > > > > committers?Some changes require attention and approval from
> >> > specific
> >> > > > > > committers. For example, changes in parts that are either very
> >> > > > > performance
> >> > > > > > sensitive, or have a critical impact on distributed
> coordination
> >> > and
> >> > > > > fault
> >> > > > > > tolerance need input by a committer that is deeply familiar
> with
> >> > the
> >> > > > > > component.As a rule of thumb, this is the case when the Pull
> >> > Request
> >> > > > > > description answers one of the questions in the template
> section
> >> > > “Does
> >> > > > > this
> >> > > > > > pull request potentially affect one of the following parts”
> with
> >> > > > > ‘yes’.This
> >> > > > > > question can be answered with - Does not need specific
> >> attention-
> >> > > Needs
> >> > > > > > specific attention for X (X can be for example checkpointing,
> >> > > > jobmanager,
> >> > > > > > etc.).- Has specific attention for X by @commiterA,
> >> @contributorBIf
> >> > > the
> >> > > > > > pull request needs specific attention, one of the tagged
> >> > > > > > committers/contributors should give the final approval.(3) Is
> >> the
> >> > > > > > contribution described well?Check whether the contribution is
> >> > > > > sufficiently
> >> > > > > > well described to support a good review. Trivial changes and
> >> fixes
> >> > do
> >> > > > not
> >> > > > > > need a long description. Any pull request that changes
> >> > functionality
> >> > > or
> >> > > > > > behavior needs to describe the big picture of these changes,
> so
> >> > that
> >> > > > > > reviews know what to look for (and don’t have to dig through
> the
> >> > code
> >> > > > to
> >> > > > > > hopefully understand what the change does).Changes that
> require
> >> > > longer
> >> > > > > > descriptions are ideally based on a prior design discussion in
> >> the
> >> > > > > mailing
> >> > > > > > list or in JIRA and can simply link to there or copy the
> >> > description
> >> > > > from
> >> > > > > > there.(4) Does the implementation follow the right overall
> >> > > > > > approach/architecture?Is this the best approach to implement
> the
> >> > fix
> >> > > or
> >> > > > > > feature, or are there other approaches that would be easier,
> >> more
> >> > > > robust,
> >> > > > > > or more maintainable?This question should be answerable from
> the
> >> > Pull
> >> > > > > > Request description (or the linked JIRA) as much as
> possible.We
> >> > > > recommend
> >> > > > > > to check this before diving into the details of commenting on
> >> > > > individual
> >> > > > > > parts of the change.(5) Is the overall code quality good,
> >> meeting
> >> > > > > standard
> >> > > > > > we want to maintain in Flink?This is the detailed code review
> of
> >> > the
> >> > > > > actual
> >> > > > > > changes, covering: - Are the changes doing what is described
> in
> >> the
> >> > > > > design
> >> > > > > > document or PR description?- Does the code follow the right
> >> > software
> >> > > > > > engineering practices? It the code correct, robust,
> >> maintainable,
> >> > > > > > testable?- Are the change performance aware, when changing a
> >> > > > performance
> >> > > > > > sensitive part?- Are the changes sufficiently covered by
> tests?-
> >> > Are
> >> > > > the
> >> > > > > > tests executing fast?- Does the code format follow Flink’s
> >> > checkstyle
> >> > > > > > pattern?- Does the code avoid to introduce additional compiler
> >> > > > > > warnings?Some code style guidelines can be found in the [Flink
> >> Code
> >> > > > Style
> >> > > > > > Page](
> https://flink.apache.org/contribute-code.html#code-style
> >> > > > > > <https://flink.apache.org/contribute-code.html#code-style
> >)Pull
> >> > > > Request
> >> > > > > > Review TemplateAdd the following checklist to the pull request
> >> > > review,
> >> > > > > > checking the boxes as the questions are answered:  - [ ]
> >> Consensus
> >> > > that
> >> > > > > the
> >> > > > > > contribution should go into to Flink  - [ ] Does not need
> >> specific
> >> > > > > > attention | Needs specific attention for X | Has attention
> for X
> >> > by Y
> >> > > > -
> >> > > > > [
> >> > > > > > ] Contribution description  - [ ] Architectural approach  - [
> ]
> >> > > Overall
> >> > > > > > code quality*
> >> > > > > >
> >> > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

tison
Hi Fabian,

I see a bot "project-bot" active on pull requests. It is some progress of
this thread?

Best,
tison.


Thomas Weise <[hidden email]> 于2018年9月19日周三 下午10:02写道:

> Follow-up regarding the PR template that pops up when opening a PR:
>
> I think what we have now is a fairly big blob of text that jumps up a bit
> unexpectedly for a first time contributor and is also cumbersome to deal
> with in the small PR description window. Perhaps we can improve it a bit:
>
> * Instead of putting all that text into the description, add it to
> website/wiki and just have a pointer in the PR, asking the contributor to
> review the guidelines before opening a PR.
> * If the questions further down can be made relevant to the context of the
> contribution, that would probably help both the contributor and the
> reviewer. For example, the questions would be different for a documentation
> change, connector change or work deep in core. Not sure if that can be
> automated, but if moved to a separate page, it could be structured better.
>
> Thanks,
> Thomas
>
>
>
>
>
>
> On Tue, Sep 18, 2018 at 8:13 AM 陈梓立 <[hidden email]> wrote:
>
> > Put some good cases here might be helpful.
> >
> > See how a contribution of runtime module be proposed, discussed,
> > implemented and merged from  https://github.com/apache/flink/pull/5931
> to
> > https://github.com/apache/flink/pull/6132.
> >
> > 1. #5931 fix a bug, but remains points could be improved. Here sihuazhou
> > and shuai-xu share their considerations and require review(of the
> proposal)
> > by Stephan, Till and Gary, our committers.
> > 2. After discussion, all people involved reach a consensus. So the rest
> > work is to implement it.
> > 3. sihuazhou gives out an implementation #6132, Till reviews it and find
> it
> > is somewhat out of the "architectural" aspect, so suggests
> > implementation-level changes.
> > 4. Addressing those implementation-level comments, the PR gets merged.
> >
> > I think this is quite a good example how we think our review process
> should
> > go.
> >
> > Best,
> > tison.
> >
> >
> > 陈梓立 <[hidden email]> 于2018年9月18日周二 下午10:53写道:
> >
> > > Maybe a little rearrange to the process would help.
> > >
> > > (1). Does the contributor describe itself well?
> > >   (1.1) By whom this contribution should be given attention. This often
> > > shows by its title, "[FLINK-XXX] [module]", the module part infer.
> > >   (1.2) What the purpose of this contribution is. Done by the PR
> > template.
> > > Even on JIRA an issue should cover these points.
> > >
> > > (2). Is there consensus on the contribution?
> > > This follows (1), because we need to clear what the purpose of the
> > > contribution first. At this stage reviewers could cc to module
> maintainer
> > > as a supplement to (1.1). Also reviewers might ask the contributor to
> > > clarify his purpose to sharp(1.2)
> > >
> > > (3). Is the implement architectural and fit code style?
> > > This follows (2). And only after a consensus we talk about concrete
> > > implement, which prevent spend time and put effort in vain.
> > >
> > > In addition, ideally a "+1" comment or approval means the purpose of
> > > contribution is supported by the reviewer and implement(if there is)
> > > quality is fine, so the reviewer vote for a consensus.
> > >
> > > Best,
> > > tison.
> > >
> > >
> > > Stephan Ewen <[hidden email]> 于2018年9月18日周二 下午6:44写道:
> > >
> > >> On the template discussion, some thoughts
> > >>
> > >> *PR Template*
> > >>
> > >> I think the PR template went well. We can rethink the "checklist" at
> the
> > >> bottom, but all other parts turned out helpful in my opinion.
> > >>
> > >> With the amount of contributions, it helps to ask the contributor to
> > take
> > >> a
> > >> little more work in order for the reviewer to be more efficient.
> > >> I would suggest to keep that mindset: Whenever we find a way that the
> > >> contributor can prepare stuff in such a way that reviews become
> > >> more efficient, we should do that. In my experience, most contributors
> > are
> > >> willing to put in some extra minutes if it helps that their
> > >> PR gets merged faster.
> > >>
> > >> *Review Template*
> > >>
> > >> I think it would be helpful to have this checklist. It does not matter
> > in
> > >> which form, be that as a text template, be that as labels.
> > >>
> > >> The most important thing is to make explicit which questions have been
> > >> answered in the review.
> > >> Currently there is a lot of "+1" on pull requests which means "code
> > >> quality
> > >> is fine", but all other questions are unanswered.
> > >> The contributors then rightfully wonder why this does not get merged.
> > >>
> > >>
> > >>
> > >> On Tue, Sep 18, 2018 at 7:26 AM, 陈梓立 <[hidden email]> wrote:
> > >>
> > >> > Hi all interested,
> > >> >
> > >> > Within the document there is a heated discussion about how the PR
> > >> > template/review template should be.
> > >> >
> > >> > Here share my opinion:
> > >> >
> > >> > 1. For the review template, actually we don't need comment a review
> > >> > template at all. GitHub has a tag system and only committer could
> add
> > >> tags,
> > >> > which we can make use of it. That is, tagging this PR is
> > >> > waiting-for-proposal-approved, waiting-for-code-review,
> > >> > waiting-for-benchmark or block-by-author and so on. Asfbot could
> pick
> > >> > GitHub tag state to the corresponding JIRA and we always regard JIRA
> > as
> > >> the
> > >> > main discussion borad.
> > >> >
> > >> > 2. For the PR template, the greeting message is redundant. Just
> > >> emphasize a
> > >> > JIRA associated is important and how to format the title is enough.
> > >> > Besides, the "Does this pull request potentially affect one of the
> > >> > following parts" part and "Documentation" should be coved from "What
> > is
> > >> the
> > >> > purpose of the change" and "Brief change log". These two parts,
> users
> > >> > always answer no and would be aware if they really make changes on
> it.
> > >> As
> > >> > example, even pull request requires document, its owner might no add
> > it
> > >> at
> > >> > first. The PR template is a guide but not which one have to learn.
> > >> >
> > >> > To sum up, (1) take advantage of GitHub's tag system to tag review
> > >> progress
> > >> > (2) make the template more concise to avoid burden mature
> contributors
> > >> and
> > >> > force new comer to learn too much.
> > >> >
> > >> > Best,
> > >> > tison.
> > >> >
> > >> >
> > >> > Rong Rong <[hidden email]> 于2018年9月18日周二 上午7:05写道:
> > >> >
> > >> > > Thanks for putting the review contribution doc together, Stephan!
> > This
> > >> > will
> > >> > > definitely help the community to make the review process better.
> > >> > >
> > >> > > From my experience this will benefit on both contributors and
> > >> reviewers
> > >> > > side! Thus +1 for putting into practice as well.
> > >> > >
> > >> > > --
> > >> > > Rong
> > >> > >
> > >> > > On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]>
> > >> wrote:
> > >> > >
> > >> > > > Hi!
> > >> > > >
> > >> > > > Thanks you for the encouraging feedback so far.
> > >> > > >
> > >> > > > The overall goal is definitely to make the contribution process
> > >> better
> > >> > > and
> > >> > > > get fewer pull requests that are disregarded.
> > >> > > >
> > >> > > > There are various reasons for the disregarded pull requests, one
> > >> being
> > >> > > that
> > >> > > > fewer committers really participate in reviews beyond
> > >> > > > the component they are currently very involved with. This is a
> > >> separate
> > >> > > > issue and I am thinking on how to encourage more
> > >> > > > activity there.
> > >> > > >
> > >> > > > The other reason I was lack of structure and lack of decision
> > >> making,
> > >> > > which
> > >> > > > is what I am first trying to fix here.
> > >> > > > A follow-up to this will definitely be to improve the
> contribution
> > >> > guide
> > >> > > as
> > >> > > > well.
> > >> > > >
> > >> > > > Best,
> > >> > > > Stephan
> > >> > > >
> > >> > > >
> > >> > > > On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
> > >> > > > [hidden email]> wrote:
> > >> > > >
> > >> > > > > From my personal experience as a contributor for three years,
> I
> > >> feel
> > >> > > > > better experience in contirbuting or reviewing than before,
> > >> although
> > >> > we
> > >> > > > > still have some points for further progress.
> > >> > > > >
> > >> > > > > I reviewed the proposal doc, and it gives very constructive
> and
> > >> > > > meaningful
> > >> > > > > guides which could help both contributor and reviewer. I agree
> > >> with
> > >> > the
> > >> > > > > bove suggestions and wish they can be praticed well!
> > >> > > > >
> > >> > > > > Best,
> > >> > > > > Zhijiang
> > >> > > > >
> > ------------------------------------------------------------------
> > >> > > > > 发件人:Till Rohrmann <[hidden email]>
> > >> > > > > 发送时间:2018年9月17日(星期一) 16:27
> > >> > > > > 收件人:dev <[hidden email]>
> > >> > > > > 主 题:Re: [PROPOSAL] [community] A more structured approach to
> > >> reviews
> > >> > > and
> > >> > > > > contributions
> > >> > > > >
> > >> > > > > Thanks for writing this up Stephan. I like the steps and hope
> > >> that it
> > >> > > > will
> > >> > > > > help the community to make the review process better. Thus, +1
> > for
> > >> > > > putting
> > >> > > > > your proposal to practice.
> > >> > > > >
> > >> > > > > Cheers,
> > >> > > > > Till
> > >> > > > >
> > >> > > > > On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <
> [hidden email]
> > >
> > >> > > wrote:
> > >> > > > >
> > >> > > > > > Hi Flink community members!
> > >> > > > > >
> > >> > > > > > As many of you will have noticed, the Flink project activity
> > has
> > >> > gone
> > >> > > > up
> > >> > > > > > again quite a bit.
> > >> > > > > > There are many more contributions, which is an absolutely
> > great
> > >> > thing
> > >> > > > to
> > >> > > > > > have :-)
> > >> > > > > >
> > >> > > > > > However, we see a continuously growing backlog of pull
> > requests
> > >> and
> > >> > > > JIRA
> > >> > > > > > issues.
> > >> > > > > > To make sure the community will be able to handle the
> > increased
> > >> > > > volume, I
> > >> > > > > > think we need to revisit some
> > >> > > > > > approaches and processes. I believe there are a few
> > >> opportunities
> > >> > to
> > >> > > > > > structure things a bit better, which
> > >> > > > > > should help to scale the development.
> > >> > > > > >
> > >> > > > > > The first thing I would like to bring up are *Pull Request
> > >> > Reviews*.
> > >> > > > Even
> > >> > > > > > though more community members being
> > >> > > > > > active in reviews (which is a really great thing!) the Pull
> > >> Request
> > >> > > > > backlog
> > >> > > > > > is increasing quite a bit.
> > >> > > > > >
> > >> > > > > > Why are pull requests still not merged faster? Looking at
> the
> > >> > > reviews,
> > >> > > > > one
> > >> > > > > > thing I noticed is that most reviews deal
> > >> > > > > > immediately with detailed code issues, and leave out most of
> > the
> > >> > core
> > >> > > > > > questions that need to be answered
> > >> > > > > > before a Pull Request can be merged, like "is this a desired
> > >> > > feature?"
> > >> > > > or
> > >> > > > > > "does this align well with other developments?".
> > >> > > > > > I think that we even make things slightly worse that way:
> From
> > >> my
> > >> > > > > personal
> > >> > > > > > experience, I have often thought "oh, this
> > >> > > > > > PR has a review already" and rather looked at another PR,
> only
> > >> to
> > >> > > find
> > >> > > > > > later that the first review did never decide whether
> > >> > > > > > this PR is actually a good fit for Flink.
> > >> > > > > >
> > >> > > > > > There has never been a proper documentation of how to answer
> > >> these
> > >> > > > > > questions, what to evaluate in reviews,
> > >> > > > > > guidelines for how to evaluate pull requests, other than
> code
> > >> > > quality.
> > >> > > > I
> > >> > > > > > suspect that this is why so many reviewers
> > >> > > > > > do not address the "is this a good contribution" questions,
> > >> making
> > >> > > pull
> > >> > > > > > requests linger until another committers joins
> > >> > > > > > the review.
> > >> > > > > >
> > >> > > > > > Below is an idea for a guide *"How to Review
> Contributions"*.
> > It
> > >> > > > outlines
> > >> > > > > > five core aspects to be checked in every
> > >> > > > > > pull request, and suggests a priority for clarifying those.
> > The
> > >> > idea
> > >> > > is
> > >> > > > > > that this helps us to better structure reviews, and
> > >> > > > > > to make each reviewer aware what we look for in a review and
> > >> where
> > >> > to
> > >> > > > > best
> > >> > > > > > bring in their help.
> > >> > > > > >
> > >> > > > > > Looking forward to comments!
> > >> > > > > >
> > >> > > > > > Best,
> > >> > > > > > Stephan
> > >> > > > > >
> > >> > > > > > ====================================
> > >> > > > > >
> > >> > > > > > The draft is in this Google Doc. Please add small textual
> > >> comments
> > >> > to
> > >> > > > the
> > >> > > > > > doc, and bigger principle discussions as replies to this
> mail.
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
> > >> > > > > RbocGlGKCYnvJd9lVhk/edit?usp=sharing
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > >
> > >> > > > > > *How to Review
> Contributions------------------------------This
> > >> > guide
> > >> > > is
> > >> > > > > for
> > >> > > > > > all committers and contributors that want to help with
> > reviewing
> > >> > > > > > contributions. Thank you for your effort - good reviews are
> > one
> > >> the
> > >> > > > most
> > >> > > > > > important and crucial parts of an open source project. This
> > >> guide
> > >> > > > should
> > >> > > > > > help the community to make reviews such that: - Contributors
> > >> have a
> > >> > > > good
> > >> > > > > > contribution experience- Reviews are structured and check
> all
> > >> > > important
> > >> > > > > > aspects of a contribution- Make sure we keep a high code
> > >> quality in
> > >> > > > > Flink-
> > >> > > > > > We avoid situations where contributors and reviewers spend a
> > >> lot of
> > >> > > > time
> > >> > > > > to
> > >> > > > > > refine a contribution that gets rejected laterReview
> > >> ChecklistEvery
> > >> > > > > review
> > >> > > > > > needs to check the following five aspects. We encourage to
> > check
> > >> > > these
> > >> > > > > > aspects in order, to avoid spending time on detailed code
> > >> quality
> > >> > > > reviews
> > >> > > > > > when there is not yet consensus that a feature or change
> > should
> > >> be
> > >> > > > > actually
> > >> > > > > > be added.(1) Is there consensus whether the change of
> feature
> > >> > should
> > >> > > go
> > >> > > > > > into to Flink?For bug fixes, this needs to be checked only
> in
> > >> case
> > >> > it
> > >> > > > > > requires bigger changes or might break existing programs and
> > >> > > > > > setups.Ideally, this question is already answered from a
> JIRA
> > >> issue
> > >> > > or
> > >> > > > > the
> > >> > > > > > dev-list discussion, except in cases of bug fixes and small
> > >> > > lightweight
> > >> > > > > > additions/extensions. In that case, this question can be
> > >> > immediately
> > >> > > > > marked
> > >> > > > > > as resolved. For pull requests that are created without
> prior
> > >> > > > consensus,
> > >> > > > > > this question needs to be answered as part of the review.The
> > >> > decision
> > >> > > > > > whether the change should go into Flink needs to take the
> > >> following
> > >> > > > > aspects
> > >> > > > > > into consideration: - Does the contribution alter the
> behavior
> > >> of
> > >> > > > > features
> > >> > > > > > or components in a way that it may break previous users’
> > >> programs
> > >> > and
> > >> > > > > > setups? If yes, there needs to be a discussion and agreement
> > >> that
> > >> > > this
> > >> > > > > > change is desirable. - Does the contribution conceptually
> fit
> > >> well
> > >> > > into
> > >> > > > > > Flink? Is it too much of special case such that it makes
> > things
> > >> > more
> > >> > > > > > complicated for the common case, or bloats the abstractions
> /
> > >> > APIs? -
> > >> > > > > Does
> > >> > > > > > the feature fit well into Flink’s architecture? Will it
> scale
> > >> and
> > >> > > keep
> > >> > > > > > Flink flexible for the future, or will the feature restrict
> > >> Flink
> > >> > in
> > >> > > > the
> > >> > > > > > future? - Is the feature a significant new addition (rather
> > >> than an
> > >> > > > > > improvement to an existing part)? If yes, will the Flink
> > >> community
> > >> > > > commit
> > >> > > > > > to maintaining this feature? - Does the feature produce
> added
> > >> value
> > >> > > for
> > >> > > > > > Flink users or developers? Or does it introduce risk of
> > >> regression
> > >> > > > > without
> > >> > > > > > adding relevant user or developer benefit?All of these
> > questions
> > >> > > should
> > >> > > > > be
> > >> > > > > > answerable from the description/discussion in JIRA and Pull
> > >> > Request,
> > >> > > > > > without looking at the code.(2) Does the contribution need
> > >> > attention
> > >> > > > from
> > >> > > > > > some specific committers and is there time commitment from
> > these
> > >> > > > > > committers?Some changes require attention and approval from
> > >> > specific
> > >> > > > > > committers. For example, changes in parts that are either
> very
> > >> > > > > performance
> > >> > > > > > sensitive, or have a critical impact on distributed
> > coordination
> > >> > and
> > >> > > > > fault
> > >> > > > > > tolerance need input by a committer that is deeply familiar
> > with
> > >> > the
> > >> > > > > > component.As a rule of thumb, this is the case when the Pull
> > >> > Request
> > >> > > > > > description answers one of the questions in the template
> > section
> > >> > > “Does
> > >> > > > > this
> > >> > > > > > pull request potentially affect one of the following parts”
> > with
> > >> > > > > ‘yes’.This
> > >> > > > > > question can be answered with - Does not need specific
> > >> attention-
> > >> > > Needs
> > >> > > > > > specific attention for X (X can be for example
> checkpointing,
> > >> > > > jobmanager,
> > >> > > > > > etc.).- Has specific attention for X by @commiterA,
> > >> @contributorBIf
> > >> > > the
> > >> > > > > > pull request needs specific attention, one of the tagged
> > >> > > > > > committers/contributors should give the final approval.(3)
> Is
> > >> the
> > >> > > > > > contribution described well?Check whether the contribution
> is
> > >> > > > > sufficiently
> > >> > > > > > well described to support a good review. Trivial changes and
> > >> fixes
> > >> > do
> > >> > > > not
> > >> > > > > > need a long description. Any pull request that changes
> > >> > functionality
> > >> > > or
> > >> > > > > > behavior needs to describe the big picture of these changes,
> > so
> > >> > that
> > >> > > > > > reviews know what to look for (and don’t have to dig through
> > the
> > >> > code
> > >> > > > to
> > >> > > > > > hopefully understand what the change does).Changes that
> > require
> > >> > > longer
> > >> > > > > > descriptions are ideally based on a prior design discussion
> in
> > >> the
> > >> > > > > mailing
> > >> > > > > > list or in JIRA and can simply link to there or copy the
> > >> > description
> > >> > > > from
> > >> > > > > > there.(4) Does the implementation follow the right overall
> > >> > > > > > approach/architecture?Is this the best approach to implement
> > the
> > >> > fix
> > >> > > or
> > >> > > > > > feature, or are there other approaches that would be easier,
> > >> more
> > >> > > > robust,
> > >> > > > > > or more maintainable?This question should be answerable from
> > the
> > >> > Pull
> > >> > > > > > Request description (or the linked JIRA) as much as
> > possible.We
> > >> > > > recommend
> > >> > > > > > to check this before diving into the details of commenting
> on
> > >> > > > individual
> > >> > > > > > parts of the change.(5) Is the overall code quality good,
> > >> meeting
> > >> > > > > standard
> > >> > > > > > we want to maintain in Flink?This is the detailed code
> review
> > of
> > >> > the
> > >> > > > > actual
> > >> > > > > > changes, covering: - Are the changes doing what is described
> > in
> > >> the
> > >> > > > > design
> > >> > > > > > document or PR description?- Does the code follow the right
> > >> > software
> > >> > > > > > engineering practices? It the code correct, robust,
> > >> maintainable,
> > >> > > > > > testable?- Are the change performance aware, when changing a
> > >> > > > performance
> > >> > > > > > sensitive part?- Are the changes sufficiently covered by
> > tests?-
> > >> > Are
> > >> > > > the
> > >> > > > > > tests executing fast?- Does the code format follow Flink’s
> > >> > checkstyle
> > >> > > > > > pattern?- Does the code avoid to introduce additional
> compiler
> > >> > > > > > warnings?Some code style guidelines can be found in the
> [Flink
> > >> Code
> > >> > > > Style
> > >> > > > > > Page](
> > https://flink.apache.org/contribute-code.html#code-style
> > >> > > > > > <https://flink.apache.org/contribute-code.html#code-style
> > >)Pull
> > >> > > > Request
> > >> > > > > > Review TemplateAdd the following checklist to the pull
> request
> > >> > > review,
> > >> > > > > > checking the boxes as the questions are answered:  - [ ]
> > >> Consensus
> > >> > > that
> > >> > > > > the
> > >> > > > > > contribution should go into to Flink  - [ ] Does not need
> > >> specific
> > >> > > > > > attention | Needs specific attention for X | Has attention
> > for X
> > >> > by Y
> > >> > > > -
> > >> > > > > [
> > >> > > > > > ] Contribution description  - [ ] Architectural approach  -
> [
> > ]
> > >> > > Overall
> > >> > > > > > code quality*
> > >> > > > > >
> > >> > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions

Stephan Ewen
Hi all!

Thank you for the interest and lively discussion.

I think we are starting to mix different things into one discussion, so I
would like to close this thread and start three separate dedicated threads:

(1) Contributing Guide and Pull Request Template

(2) Review Process (steps, questions, checklists only, no tools like
bots/labels/templates)

(3) Review Tools (whether to drive the review process with labels,
comments/templates, bots, etc.)

Will post these threads asap.

Best,
Stephan


On Thu, Sep 20, 2018 at 3:49 PM, 陈梓立 <[hidden email]> wrote:

> Hi Fabian,
>
> I see a bot "project-bot" active on pull requests. It is some progress of
> this thread?
>
> Best,
> tison.
>
>
> Thomas Weise <[hidden email]> 于2018年9月19日周三 下午10:02写道:
>
> > Follow-up regarding the PR template that pops up when opening a PR:
> >
> > I think what we have now is a fairly big blob of text that jumps up a bit
> > unexpectedly for a first time contributor and is also cumbersome to deal
> > with in the small PR description window. Perhaps we can improve it a bit:
> >
> > * Instead of putting all that text into the description, add it to
> > website/wiki and just have a pointer in the PR, asking the contributor to
> > review the guidelines before opening a PR.
> > * If the questions further down can be made relevant to the context of
> the
> > contribution, that would probably help both the contributor and the
> > reviewer. For example, the questions would be different for a
> documentation
> > change, connector change or work deep in core. Not sure if that can be
> > automated, but if moved to a separate page, it could be structured
> better.
> >
> > Thanks,
> > Thomas
> >
> >
> >
> >
> >
> >
> > On Tue, Sep 18, 2018 at 8:13 AM 陈梓立 <[hidden email]> wrote:
> >
> > > Put some good cases here might be helpful.
> > >
> > > See how a contribution of runtime module be proposed, discussed,
> > > implemented and merged from  https://github.com/apache/flink/pull/5931
> > to
> > > https://github.com/apache/flink/pull/6132.
> > >
> > > 1. #5931 fix a bug, but remains points could be improved. Here
> sihuazhou
> > > and shuai-xu share their considerations and require review(of the
> > proposal)
> > > by Stephan, Till and Gary, our committers.
> > > 2. After discussion, all people involved reach a consensus. So the rest
> > > work is to implement it.
> > > 3. sihuazhou gives out an implementation #6132, Till reviews it and
> find
> > it
> > > is somewhat out of the "architectural" aspect, so suggests
> > > implementation-level changes.
> > > 4. Addressing those implementation-level comments, the PR gets merged.
> > >
> > > I think this is quite a good example how we think our review process
> > should
> > > go.
> > >
> > > Best,
> > > tison.
> > >
> > >
> > > 陈梓立 <[hidden email]> 于2018年9月18日周二 下午10:53写道:
> > >
> > > > Maybe a little rearrange to the process would help.
> > > >
> > > > (1). Does the contributor describe itself well?
> > > >   (1.1) By whom this contribution should be given attention. This
> often
> > > > shows by its title, "[FLINK-XXX] [module]", the module part infer.
> > > >   (1.2) What the purpose of this contribution is. Done by the PR
> > > template.
> > > > Even on JIRA an issue should cover these points.
> > > >
> > > > (2). Is there consensus on the contribution?
> > > > This follows (1), because we need to clear what the purpose of the
> > > > contribution first. At this stage reviewers could cc to module
> > maintainer
> > > > as a supplement to (1.1). Also reviewers might ask the contributor to
> > > > clarify his purpose to sharp(1.2)
> > > >
> > > > (3). Is the implement architectural and fit code style?
> > > > This follows (2). And only after a consensus we talk about concrete
> > > > implement, which prevent spend time and put effort in vain.
> > > >
> > > > In addition, ideally a "+1" comment or approval means the purpose of
> > > > contribution is supported by the reviewer and implement(if there is)
> > > > quality is fine, so the reviewer vote for a consensus.
> > > >
> > > > Best,
> > > > tison.
> > > >
> > > >
> > > > Stephan Ewen <[hidden email]> 于2018年9月18日周二 下午6:44写道:
> > > >
> > > >> On the template discussion, some thoughts
> > > >>
> > > >> *PR Template*
> > > >>
> > > >> I think the PR template went well. We can rethink the "checklist" at
> > the
> > > >> bottom, but all other parts turned out helpful in my opinion.
> > > >>
> > > >> With the amount of contributions, it helps to ask the contributor to
> > > take
> > > >> a
> > > >> little more work in order for the reviewer to be more efficient.
> > > >> I would suggest to keep that mindset: Whenever we find a way that
> the
> > > >> contributor can prepare stuff in such a way that reviews become
> > > >> more efficient, we should do that. In my experience, most
> contributors
> > > are
> > > >> willing to put in some extra minutes if it helps that their
> > > >> PR gets merged faster.
> > > >>
> > > >> *Review Template*
> > > >>
> > > >> I think it would be helpful to have this checklist. It does not
> matter
> > > in
> > > >> which form, be that as a text template, be that as labels.
> > > >>
> > > >> The most important thing is to make explicit which questions have
> been
> > > >> answered in the review.
> > > >> Currently there is a lot of "+1" on pull requests which means "code
> > > >> quality
> > > >> is fine", but all other questions are unanswered.
> > > >> The contributors then rightfully wonder why this does not get
> merged.
> > > >>
> > > >>
> > > >>
> > > >> On Tue, Sep 18, 2018 at 7:26 AM, 陈梓立 <[hidden email]> wrote:
> > > >>
> > > >> > Hi all interested,
> > > >> >
> > > >> > Within the document there is a heated discussion about how the PR
> > > >> > template/review template should be.
> > > >> >
> > > >> > Here share my opinion:
> > > >> >
> > > >> > 1. For the review template, actually we don't need comment a
> review
> > > >> > template at all. GitHub has a tag system and only committer could
> > add
> > > >> tags,
> > > >> > which we can make use of it. That is, tagging this PR is
> > > >> > waiting-for-proposal-approved, waiting-for-code-review,
> > > >> > waiting-for-benchmark or block-by-author and so on. Asfbot could
> > pick
> > > >> > GitHub tag state to the corresponding JIRA and we always regard
> JIRA
> > > as
> > > >> the
> > > >> > main discussion borad.
> > > >> >
> > > >> > 2. For the PR template, the greeting message is redundant. Just
> > > >> emphasize a
> > > >> > JIRA associated is important and how to format the title is
> enough.
> > > >> > Besides, the "Does this pull request potentially affect one of the
> > > >> > following parts" part and "Documentation" should be coved from
> "What
> > > is
> > > >> the
> > > >> > purpose of the change" and "Brief change log". These two parts,
> > users
> > > >> > always answer no and would be aware if they really make changes on
> > it.
> > > >> As
> > > >> > example, even pull request requires document, its owner might no
> add
> > > it
> > > >> at
> > > >> > first. The PR template is a guide but not which one have to learn.
> > > >> >
> > > >> > To sum up, (1) take advantage of GitHub's tag system to tag review
> > > >> progress
> > > >> > (2) make the template more concise to avoid burden mature
> > contributors
> > > >> and
> > > >> > force new comer to learn too much.
> > > >> >
> > > >> > Best,
> > > >> > tison.
> > > >> >
> > > >> >
> > > >> > Rong Rong <[hidden email]> 于2018年9月18日周二 上午7:05写道:
> > > >> >
> > > >> > > Thanks for putting the review contribution doc together,
> Stephan!
> > > This
> > > >> > will
> > > >> > > definitely help the community to make the review process better.
> > > >> > >
> > > >> > > From my experience this will benefit on both contributors and
> > > >> reviewers
> > > >> > > side! Thus +1 for putting into practice as well.
> > > >> > >
> > > >> > > --
> > > >> > > Rong
> > > >> > >
> > > >> > > On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <[hidden email]
> >
> > > >> wrote:
> > > >> > >
> > > >> > > > Hi!
> > > >> > > >
> > > >> > > > Thanks you for the encouraging feedback so far.
> > > >> > > >
> > > >> > > > The overall goal is definitely to make the contribution
> process
> > > >> better
> > > >> > > and
> > > >> > > > get fewer pull requests that are disregarded.
> > > >> > > >
> > > >> > > > There are various reasons for the disregarded pull requests,
> one
> > > >> being
> > > >> > > that
> > > >> > > > fewer committers really participate in reviews beyond
> > > >> > > > the component they are currently very involved with. This is a
> > > >> separate
> > > >> > > > issue and I am thinking on how to encourage more
> > > >> > > > activity there.
> > > >> > > >
> > > >> > > > The other reason I was lack of structure and lack of decision
> > > >> making,
> > > >> > > which
> > > >> > > > is what I am first trying to fix here.
> > > >> > > > A follow-up to this will definitely be to improve the
> > contribution
> > > >> > guide
> > > >> > > as
> > > >> > > > well.
> > > >> > > >
> > > >> > > > Best,
> > > >> > > > Stephan
> > > >> > > >
> > > >> > > >
> > > >> > > > On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) <
> > > >> > > > [hidden email]> wrote:
> > > >> > > >
> > > >> > > > > From my personal experience as a contributor for three
> years,
> > I
> > > >> feel
> > > >> > > > > better experience in contirbuting or reviewing than before,
> > > >> although
> > > >> > we
> > > >> > > > > still have some points for further progress.
> > > >> > > > >
> > > >> > > > > I reviewed the proposal doc, and it gives very constructive
> > and
> > > >> > > > meaningful
> > > >> > > > > guides which could help both contributor and reviewer. I
> agree
> > > >> with
> > > >> > the
> > > >> > > > > bove suggestions and wish they can be praticed well!
> > > >> > > > >
> > > >> > > > > Best,
> > > >> > > > > Zhijiang
> > > >> > > > >
> > > ------------------------------------------------------------------
> > > >> > > > > 发件人:Till Rohrmann <[hidden email]>
> > > >> > > > > 发送时间:2018年9月17日(星期一) 16:27
> > > >> > > > > 收件人:dev <[hidden email]>
> > > >> > > > > 主 题:Re: [PROPOSAL] [community] A more structured approach to
> > > >> reviews
> > > >> > > and
> > > >> > > > > contributions
> > > >> > > > >
> > > >> > > > > Thanks for writing this up Stephan. I like the steps and
> hope
> > > >> that it
> > > >> > > > will
> > > >> > > > > help the community to make the review process better. Thus,
> +1
> > > for
> > > >> > > > putting
> > > >> > > > > your proposal to practice.
> > > >> > > > >
> > > >> > > > > Cheers,
> > > >> > > > > Till
> > > >> > > > >
> > > >> > > > > On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen <
> > [hidden email]
> > > >
> > > >> > > wrote:
> > > >> > > > >
> > > >> > > > > > Hi Flink community members!
> > > >> > > > > >
> > > >> > > > > > As many of you will have noticed, the Flink project
> activity
> > > has
> > > >> > gone
> > > >> > > > up
> > > >> > > > > > again quite a bit.
> > > >> > > > > > There are many more contributions, which is an absolutely
> > > great
> > > >> > thing
> > > >> > > > to
> > > >> > > > > > have :-)
> > > >> > > > > >
> > > >> > > > > > However, we see a continuously growing backlog of pull
> > > requests
> > > >> and
> > > >> > > > JIRA
> > > >> > > > > > issues.
> > > >> > > > > > To make sure the community will be able to handle the
> > > increased
> > > >> > > > volume, I
> > > >> > > > > > think we need to revisit some
> > > >> > > > > > approaches and processes. I believe there are a few
> > > >> opportunities
> > > >> > to
> > > >> > > > > > structure things a bit better, which
> > > >> > > > > > should help to scale the development.
> > > >> > > > > >
> > > >> > > > > > The first thing I would like to bring up are *Pull Request
> > > >> > Reviews*.
> > > >> > > > Even
> > > >> > > > > > though more community members being
> > > >> > > > > > active in reviews (which is a really great thing!) the
> Pull
> > > >> Request
> > > >> > > > > backlog
> > > >> > > > > > is increasing quite a bit.
> > > >> > > > > >
> > > >> > > > > > Why are pull requests still not merged faster? Looking at
> > the
> > > >> > > reviews,
> > > >> > > > > one
> > > >> > > > > > thing I noticed is that most reviews deal
> > > >> > > > > > immediately with detailed code issues, and leave out most
> of
> > > the
> > > >> > core
> > > >> > > > > > questions that need to be answered
> > > >> > > > > > before a Pull Request can be merged, like "is this a
> desired
> > > >> > > feature?"
> > > >> > > > or
> > > >> > > > > > "does this align well with other developments?".
> > > >> > > > > > I think that we even make things slightly worse that way:
> > From
> > > >> my
> > > >> > > > > personal
> > > >> > > > > > experience, I have often thought "oh, this
> > > >> > > > > > PR has a review already" and rather looked at another PR,
> > only
> > > >> to
> > > >> > > find
> > > >> > > > > > later that the first review did never decide whether
> > > >> > > > > > this PR is actually a good fit for Flink.
> > > >> > > > > >
> > > >> > > > > > There has never been a proper documentation of how to
> answer
> > > >> these
> > > >> > > > > > questions, what to evaluate in reviews,
> > > >> > > > > > guidelines for how to evaluate pull requests, other than
> > code
> > > >> > > quality.
> > > >> > > > I
> > > >> > > > > > suspect that this is why so many reviewers
> > > >> > > > > > do not address the "is this a good contribution"
> questions,
> > > >> making
> > > >> > > pull
> > > >> > > > > > requests linger until another committers joins
> > > >> > > > > > the review.
> > > >> > > > > >
> > > >> > > > > > Below is an idea for a guide *"How to Review
> > Contributions"*.
> > > It
> > > >> > > > outlines
> > > >> > > > > > five core aspects to be checked in every
> > > >> > > > > > pull request, and suggests a priority for clarifying
> those.
> > > The
> > > >> > idea
> > > >> > > is
> > > >> > > > > > that this helps us to better structure reviews, and
> > > >> > > > > > to make each reviewer aware what we look for in a review
> and
> > > >> where
> > > >> > to
> > > >> > > > > best
> > > >> > > > > > bring in their help.
> > > >> > > > > >
> > > >> > > > > > Looking forward to comments!
> > > >> > > > > >
> > > >> > > > > > Best,
> > > >> > > > > > Stephan
> > > >> > > > > >
> > > >> > > > > > ====================================
> > > >> > > > > >
> > > >> > > > > > The draft is in this Google Doc. Please add small textual
> > > >> comments
> > > >> > to
> > > >> > > > the
> > > >> > > > > > doc, and bigger principle discussions as replies to this
> > mail.
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c
> > > >> > > > > RbocGlGKCYnvJd9lVhk/edit?usp=sharing
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > > *How to Review
> > Contributions------------------------------This
> > > >> > guide
> > > >> > > is
> > > >> > > > > for
> > > >> > > > > > all committers and contributors that want to help with
> > > reviewing
> > > >> > > > > > contributions. Thank you for your effort - good reviews
> are
> > > one
> > > >> the
> > > >> > > > most
> > > >> > > > > > important and crucial parts of an open source project.
> This
> > > >> guide
> > > >> > > > should
> > > >> > > > > > help the community to make reviews such that: -
> Contributors
> > > >> have a
> > > >> > > > good
> > > >> > > > > > contribution experience- Reviews are structured and check
> > all
> > > >> > > important
> > > >> > > > > > aspects of a contribution- Make sure we keep a high code
> > > >> quality in
> > > >> > > > > Flink-
> > > >> > > > > > We avoid situations where contributors and reviewers
> spend a
> > > >> lot of
> > > >> > > > time
> > > >> > > > > to
> > > >> > > > > > refine a contribution that gets rejected laterReview
> > > >> ChecklistEvery
> > > >> > > > > review
> > > >> > > > > > needs to check the following five aspects. We encourage to
> > > check
> > > >> > > these
> > > >> > > > > > aspects in order, to avoid spending time on detailed code
> > > >> quality
> > > >> > > > reviews
> > > >> > > > > > when there is not yet consensus that a feature or change
> > > should
> > > >> be
> > > >> > > > > actually
> > > >> > > > > > be added.(1) Is there consensus whether the change of
> > feature
> > > >> > should
> > > >> > > go
> > > >> > > > > > into to Flink?For bug fixes, this needs to be checked only
> > in
> > > >> case
> > > >> > it
> > > >> > > > > > requires bigger changes or might break existing programs
> and
> > > >> > > > > > setups.Ideally, this question is already answered from a
> > JIRA
> > > >> issue
> > > >> > > or
> > > >> > > > > the
> > > >> > > > > > dev-list discussion, except in cases of bug fixes and
> small
> > > >> > > lightweight
> > > >> > > > > > additions/extensions. In that case, this question can be
> > > >> > immediately
> > > >> > > > > marked
> > > >> > > > > > as resolved. For pull requests that are created without
> > prior
> > > >> > > > consensus,
> > > >> > > > > > this question needs to be answered as part of the
> review.The
> > > >> > decision
> > > >> > > > > > whether the change should go into Flink needs to take the
> > > >> following
> > > >> > > > > aspects
> > > >> > > > > > into consideration: - Does the contribution alter the
> > behavior
> > > >> of
> > > >> > > > > features
> > > >> > > > > > or components in a way that it may break previous users’
> > > >> programs
> > > >> > and
> > > >> > > > > > setups? If yes, there needs to be a discussion and
> agreement
> > > >> that
> > > >> > > this
> > > >> > > > > > change is desirable. - Does the contribution conceptually
> > fit
> > > >> well
> > > >> > > into
> > > >> > > > > > Flink? Is it too much of special case such that it makes
> > > things
> > > >> > more
> > > >> > > > > > complicated for the common case, or bloats the
> abstractions
> > /
> > > >> > APIs? -
> > > >> > > > > Does
> > > >> > > > > > the feature fit well into Flink’s architecture? Will it
> > scale
> > > >> and
> > > >> > > keep
> > > >> > > > > > Flink flexible for the future, or will the feature
> restrict
> > > >> Flink
> > > >> > in
> > > >> > > > the
> > > >> > > > > > future? - Is the feature a significant new addition
> (rather
> > > >> than an
> > > >> > > > > > improvement to an existing part)? If yes, will the Flink
> > > >> community
> > > >> > > > commit
> > > >> > > > > > to maintaining this feature? - Does the feature produce
> > added
> > > >> value
> > > >> > > for
> > > >> > > > > > Flink users or developers? Or does it introduce risk of
> > > >> regression
> > > >> > > > > without
> > > >> > > > > > adding relevant user or developer benefit?All of these
> > > questions
> > > >> > > should
> > > >> > > > > be
> > > >> > > > > > answerable from the description/discussion in JIRA and
> Pull
> > > >> > Request,
> > > >> > > > > > without looking at the code.(2) Does the contribution need
> > > >> > attention
> > > >> > > > from
> > > >> > > > > > some specific committers and is there time commitment from
> > > these
> > > >> > > > > > committers?Some changes require attention and approval
> from
> > > >> > specific
> > > >> > > > > > committers. For example, changes in parts that are either
> > very
> > > >> > > > > performance
> > > >> > > > > > sensitive, or have a critical impact on distributed
> > > coordination
> > > >> > and
> > > >> > > > > fault
> > > >> > > > > > tolerance need input by a committer that is deeply
> familiar
> > > with
> > > >> > the
> > > >> > > > > > component.As a rule of thumb, this is the case when the
> Pull
> > > >> > Request
> > > >> > > > > > description answers one of the questions in the template
> > > section
> > > >> > > “Does
> > > >> > > > > this
> > > >> > > > > > pull request potentially affect one of the following
> parts”
> > > with
> > > >> > > > > ‘yes’.This
> > > >> > > > > > question can be answered with - Does not need specific
> > > >> attention-
> > > >> > > Needs
> > > >> > > > > > specific attention for X (X can be for example
> > checkpointing,
> > > >> > > > jobmanager,
> > > >> > > > > > etc.).- Has specific attention for X by @commiterA,
> > > >> @contributorBIf
> > > >> > > the
> > > >> > > > > > pull request needs specific attention, one of the tagged
> > > >> > > > > > committers/contributors should give the final approval.(3)
> > Is
> > > >> the
> > > >> > > > > > contribution described well?Check whether the contribution
> > is
> > > >> > > > > sufficiently
> > > >> > > > > > well described to support a good review. Trivial changes
> and
> > > >> fixes
> > > >> > do
> > > >> > > > not
> > > >> > > > > > need a long description. Any pull request that changes
> > > >> > functionality
> > > >> > > or
> > > >> > > > > > behavior needs to describe the big picture of these
> changes,
> > > so
> > > >> > that
> > > >> > > > > > reviews know what to look for (and don’t have to dig
> through
> > > the
> > > >> > code
> > > >> > > > to
> > > >> > > > > > hopefully understand what the change does).Changes that
> > > require
> > > >> > > longer
> > > >> > > > > > descriptions are ideally based on a prior design
> discussion
> > in
> > > >> the
> > > >> > > > > mailing
> > > >> > > > > > list or in JIRA and can simply link to there or copy the
> > > >> > description
> > > >> > > > from
> > > >> > > > > > there.(4) Does the implementation follow the right overall
> > > >> > > > > > approach/architecture?Is this the best approach to
> implement
> > > the
> > > >> > fix
> > > >> > > or
> > > >> > > > > > feature, or are there other approaches that would be
> easier,
> > > >> more
> > > >> > > > robust,
> > > >> > > > > > or more maintainable?This question should be answerable
> from
> > > the
> > > >> > Pull
> > > >> > > > > > Request description (or the linked JIRA) as much as
> > > possible.We
> > > >> > > > recommend
> > > >> > > > > > to check this before diving into the details of commenting
> > on
> > > >> > > > individual
> > > >> > > > > > parts of the change.(5) Is the overall code quality good,
> > > >> meeting
> > > >> > > > > standard
> > > >> > > > > > we want to maintain in Flink?This is the detailed code
> > review
> > > of
> > > >> > the
> > > >> > > > > actual
> > > >> > > > > > changes, covering: - Are the changes doing what is
> described
> > > in
> > > >> the
> > > >> > > > > design
> > > >> > > > > > document or PR description?- Does the code follow the
> right
> > > >> > software
> > > >> > > > > > engineering practices? It the code correct, robust,
> > > >> maintainable,
> > > >> > > > > > testable?- Are the change performance aware, when
> changing a
> > > >> > > > performance
> > > >> > > > > > sensitive part?- Are the changes sufficiently covered by
> > > tests?-
> > > >> > Are
> > > >> > > > the
> > > >> > > > > > tests executing fast?- Does the code format follow Flink’s
> > > >> > checkstyle
> > > >> > > > > > pattern?- Does the code avoid to introduce additional
> > compiler
> > > >> > > > > > warnings?Some code style guidelines can be found in the
> > [Flink
> > > >> Code
> > > >> > > > Style
> > > >> > > > > > Page](
> > > https://flink.apache.org/contribute-code.html#code-style
> > > >> > > > > > <https://flink.apache.org/contribute-code.html#code-style
> > > >)Pull
> > > >> > > > Request
> > > >> > > > > > Review TemplateAdd the following checklist to the pull
> > request
> > > >> > > review,
> > > >> > > > > > checking the boxes as the questions are answered:  - [ ]
> > > >> Consensus
> > > >> > > that
> > > >> > > > > the
> > > >> > > > > > contribution should go into to Flink  - [ ] Does not need
> > > >> specific
> > > >> > > > > > attention | Needs specific attention for X | Has attention
> > > for X
> > > >> > by Y
> > > >> > > > -
> > > >> > > > > [
> > > >> > > > > > ] Contribution description  - [ ] Architectural approach
> -
> > [
> > > ]
> > > >> > > Overall
> > > >> > > > > > code quality*
> > > >> > > > > >
> > > >> > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> >
> > > >>
> > > >
> > >
> >
>