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 *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>)*
