Hey, I agree that we need to organize the PR process. A PR management tool would be great.
However, it seems to me that the shepherding process described is -more or less- what we've already been doing. There is usually a person that reviews the PR and kind-of drives the process. Maybe making this explicit will make things go faster. There is a problem I see here, quite related to what Theo also mentioned. For how long do we let a PR lingering around without a shepherd? What do we do if nobody volunteers? Could we maybe do a "PR overall status assessment" once per week or so, where we find those problematic PRs and try to assign them / close them? Finally, I think shepherding one's own PR is a bad idea (the review part) unless it's something trivial. Cheers and see you very soon, -Vasia. On Oct 7, 2015 11:24 AM, "Matthias J. Sax" <mj...@apache.org> wrote: > Ok. That makes sense. So most people will need to change behavior and > start discussions in JIRA and not over dev list. Furthermore, issues > list must be monitored more carefully... (I personally, watch dev > carefully and only skip over issues list right now) > > -Matthias > > On 10/07/2015 10:22 AM, Fabian Hueske 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 > >>>> > >>> > >>> > >> > > > >
