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 <trohrm...@apache.org> 发送时间：2018年9月17日(星期一) 16:27 收件人：dev <dev@flink.apache.org> 主 题：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 <se...@apache.org> 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* >
