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 > > > >>>> > > > >>> > > > >>> > > > >> > > > > > > > > > > > > >
