One suggestion from me: in GitHub you can make clear who the current sheppard is through the "Assignee" field in the PR (which can and IMHO should be different from the user who actually opened the request).
Regards, A. 2015-10-16 15:58 GMT+02:00 Fabian Hueske <fhue...@gmail.com>: > Hi folks, > > I think we can split of the discussion of a PR meeting. > > Are there any more comments on the proposal itself? > Otherwise, I would go ahead and put the described process (modulo the > comments) into a document for our website. > > Cheers, Fabian > > 2015-10-07 16:57 GMT+02:00 Fabian Hueske <fhue...@gmail.com>: > > > I like the idea of meeting once a week to discuss about PRs as well. > > > > Regarding lingering PRs, you can simply sort the Flink PRs in Github by > > "least recently updated" > > > > Cheers, > > Fabian > > > > 2015-10-07 16:48 GMT+02:00 Theodore Vasiloudis < > > theodoros.vasilou...@gmail.com>: > > > >> > > >> > 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? > >> > >> > >> I like this idea, as it would raise awareness about lingering PRs. Does > >> anybody know if there is > >> some way to integrate this into JIRA, so we can easily see (and > >> filter/sort) lingering PRs? > >> > >> Cheers, > >> Theo > >> > >> On Wed, Oct 7, 2015 at 12:04 PM, Vasiliki Kalavri < > >> vasilikikala...@gmail.com > >> > wrote: > >> > >> > 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 > >> > > >>>> > >> > > >>> > >> > > >>> > >> > > >> > >> > > > > >> > > > >> > > > >> > > >> > > > > >
