osdir.com


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


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 <sewen@xxxxxxxxxx> 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) <
> wangzhijiang999@xxxxxxxxxx.invalid> 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 <trohrmann@xxxxxxxxxx>
> > 发送时间:2018年9月17日(星期一) 16:27
> > 收件人:dev <dev@xxxxxxxxxxxxxxxx>
> > 主 题: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 <sewen@xxxxxxxxxx> 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*
> > >
> >
> >
>