+1 One minor comment: I suppose you implicitly mean that a committer can shepherd her own PR.
On Wed, Oct 7, 2015 at 10:22 AM, Fabian Hueske <fhue...@gmail.com> wrote: > @Matthias: That's a good point. Each PR should be backed by a JIRA issue. > If that's not the case, we have to make the contributor aware of that rule. > I'll update the process to keep all discussions in JIRA (will be mirrored > to issues ML), OK? > > @Theo: You are right. Adding this process won't be the silver bullet to fix > all PR related issues. > But I hope it will help to improve the overall situation. > > Are there any other comment? Otherwise I would start to prepare add this to > our website. > > Thanks, Fabian > > 2015-10-06 9:46 GMT+02:00 Theodore Vasiloudis < > theodoros.vasilou...@gmail.com>: > > > 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 > > > > > > > > > > > > >
