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


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.


陈梓立 <wander4096@xxxxxxxxx> 于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 <sewen@xxxxxxxxxx> 于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, 陈梓立 <wander4096@xxxxxxxxx> 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 <walterddr@xxxxxxxxx> 于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 <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*
>> > > > > >
>> > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>