Thanks for separating the threads Stephan! (1) Do we agree on the five basic steps below?* +1 to the five steps and making the third question in the proposal the first.
(2) How do we understand that consensus is reached about adding the feature? +1 to lazy consensus with one committer's +1 (3) To answer the question whether a PR needs special attention I think this question can be answered by the committer who accepts the proposed change (question 1 of the proposal). IMO, we can add a page about component experts if we find that we need it. Cheers, Fabian Am Do., 20. Sep. 2018 um 21:53 Uhr schrieb Stephan Ewen <se...@apache.org>: > Hi all! > > This thread is dedicated to discuss the specific review steps and answers > we want to have during reviews. > It is spun out of the proposal *"A more structured approach to reviews and > contributions".* > > Please keep this thread focused on the review steps, NOT on the tooling > (bot, comment/template, labels, ...). There will be a separate thread for > that. > > > *Discussion do far* > > There seems to be almost consensus in the basic approach, with open > questions about details as outlined below. > > > *(1) Do we agree on the five basic steps below?* > > - Do we want to make "(3) Is the contribution described well" the first > item? > > > *(2) How do we understand that consensus is reached about adding the > feature?* > > - When one committer +1s the question and no other person voices > concerns, is this consensus? (classical lazy consensus) > > *(3) To answer the question whether a PR needs special attention* > > - Also tagged by the committers that drive the "should this be added" > consensus > - Should we create a wiki page of "component experts"? > > > > > *Original Review Guide Proposal* > > > https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2cRbocGlGKCYnvJd9l > Vhk/edit?usp=sharing > <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>)* >
