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