This guide is for all committers and contributors that want to help with reviewing code 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:
Every 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 no consensus yet whether a feature or change should actually be added.
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.
A contribution is well-described if the following questions 2, 3, and 4 can be answered without looking at the code.
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 can be directly answered from a Jira issue or a 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:
All of these questions should be answerable from the description/discussion in Jira and Pull Request, without looking at the code.
A feature, improvement, or bug fix is approved once one committer accepts it and no committer disagrees (lazy consensus).
In case of diverging opinions, the discussion should be moved to the respective Jira issue or to the dev mailing list and continued until consensus is reached. If the change is proposed by a committer, it is best-practice to seek the approval of another committer.
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, special attention is required 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
If the pull request needs specific attention, one of the tagged committers/contributors should give the final approval.
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.
This is the detailed code review of the actual changes, covering:
Some code style guidelines can be found in the Flink Code Style Page
The Flink community is using a service called @flinkbot to help with the review of the pull requests.
The bot automatically posts a comment tracking the review progress for each new pull request:
### Review Progress * [ ] 1. The description looks good. * [ ] 2. There is consensus that the contribution should go into to Flink. * [ ] 3. [Does not need specific attention | Needs specific attention for X | Has attention for X by Y] * [ ] 4. The architectural approach is sound. * [ ] 5. Overall code quality is good. Please see the [Pull Request Review Guide](https://flink.apache.org/reviewing-prs.html) if you have questions about the review process.
Reviewers can instruct the bot to tick off the boxes (in order) to indicate the progress of the review.
For approving the description of the contribution, mention the bot with
@flinkbot approve description. This works similarly with
For approving all aspects, put a new comment with
@flinkbot approve all into the pull request.
The syntax for requiring attention is
@flinkbot attention @username1 [@username2 ..].