flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthias J. Sax" <mj...@apache.org>
Subject Re: [DISCUSS] Introducing a review process for pull requests
Date Mon, 05 Oct 2015 13:28:23 GMT
One comment:
We should ensure that contributors follow discussions on the dev mailing
list. Otherwise, they might miss important discussions regarding their
PR (what happened already). Thus, the contributor was waiting for
feedback on the PR, while the reviewer(s) waited for the PR to be
updated according to the discussion consensus, resulting in unnecessary
delays.

-Matthias



On 10/05/2015 02:18 PM, Fabian Hueske wrote:
> Hi everybody,
> 
> Along with our efforts to improve the “How to contribute” guide, I would
> like to start a discussion about a setting up a review process for pull
> requests.
> 
> Right now, I feel that our PR review efforts are often a bit unorganized.
> This leads to situation such as:
> 
> - PRs which are lingering around without review or feedback
> - PRs which got a +1 for merging but which did not get merged
> - PRs which have been rejected after a long time
> - PRs which became irrelevant because some component was rewritten
> - PRs which are lingering around and have been abandoned by their
> contributors
> 
> To address these issues I propose to define a pull request review process
> as follows:
> 
> 1. [Get a Shepherd] Each pull request is taken care of by a shepherd.
> Shepherds are committers that voluntarily sign up and *feel responsible*
> for helping the PR through the process until it is merged (or discarded).
> The shepherd is also the person to contact for the author of the PR. A
> committer becomes the shepherd of a PR by dropping a comment on Github like
> “I would like to shepherd this PR”. A PR can be reassigned with lazy
> consensus.
> 
> 2. [Accept Decision] The first decision for a PR should be whether it is
> accepted or not. This depends on a) whether it is a desired feature or
> improvement for Flink and b) whether the higher-level solution design is
> appropriate. In many cases such as bug fixes or discussed features or
> improvements, this should be an easy decision. In case of more a complex
> feature, the discussion should have been started when the mandatory JIRA
> was created. If it is still not clear whether the PR should be accepted or
> not, a discussion should be started in JIRA (a JIRA issue needs to be
> created if none exists). The acceptance decision should be recorded by a
> “+1 to accept” message in Github. If the PR is not accepted, it should be
> closed in a timely manner.
> 
> 3. [Merge Decision] Once a PR has been “accepted”, it should be brought
> into a mergeable state. This means the community should quickly react on
> contributor questions or PR updates. Everybody is encouraged to review pull
> requests and give feedback. Ideally, the PR author does not have to wait
> for a long time to get feedback. The shepherd of a PR should feel
> responsible to drive this process forward. Once the PR is considered to be
> mergeable, this should be recorded by a “+1 to merge” message in Github. If
> the pull request becomes abandoned at some point in time, it should be
> either taken over by somebody else or be closed after a reasonable time.
> Again, this can be done by anybody, but the shepherd should feel
> responsible to resolve the issue.
> 
> 4. Once, the PR is in a mergeable state, it should be merged. This can be
> done by anybody, however the shepherd should make sure that it happens in a
> timely manner.
> 
> IMPORTANT: Everybody is encouraged to discuss pull requests, give feedback,
> and merge pull requests which are in a good shape. However, the shepherd
> should feel responsible to drive a PR forward if nothing happens.
> 
> By defining a review process for pull requests, I hope we can
> 
> - Improve the satisfaction of and interaction with contributors
> - Improve and speed-up the review process of pull requests.
> - Close irrelevant and stale PRs more timely
> - Reduce the effort for code reviewing
> 
> The review process can be supported by some tooling:
> 
> - A QA bot that checks the quality of pull requests such as increase of
> compiler warnings, code style, API changes, etc.
> - A PR management tool such as Sparks PR dashboard (see:
> https://spark-prs.appspot.com/)
> 
> I would like to start discussions about using such tools later as separate
> discussions.
> 
> Looking forward to your feedback,
> Fabian
> 


Mime
View raw message