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


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 <piotr@xxxxxxxxxxxxxxxxx> 于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, 陈梓立 <wander4096@xxxxxxxxx> 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.
> >
> >
> > 陈梓立 <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*
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
>
>