One problem that we are seeing with FlinkML PRs is that there are simply not enough commiters to "shepherd" all of them.
While I think this process would help generally, I don't think it would solve this kind of problem. Regards, Theodore On Mon, Oct 5, 2015 at 3:28 PM, Matthias J. Sax <mj...@apache.org> wrote: > 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 > > > >
