Hi everybody, I created a wiki page for our Pull Request Process. [1] Please review, refine, and comment.
I would suggest to start following the process once 0.10 is released. What do you think? Cheers, Fabian [1] https://cwiki.apache.org/confluence/display/FLINK/Pull+Request+Management 2015-10-19 17:53 GMT+02:00 Fabian Hueske <fhue...@gmail.com>: > Thanks for your feedback, Alex. > > Chesnay is right, we cannot modify the GH assignee field at the moment. If > this changes at some point, I would support your proposal. > > Regarding the PR - JIRA rule, this has been discussed as part of the new > contribution guidelines discussion [1]. > I agree, it is not always easy to draw a line here. However, if in doubt I > would go for the JIRA issue because it allows everybody to track the issue > later, esp. if it solves a bug that other people might run into as well. > In your concrete example, you could have referenced the original JIRA > issue to remove Spargel from your PR. > > I also agree that the shepherd should not be the author of the PR. > However, every committer can always commit to the master branch (unless > somebody raised concerns of course). So committers should be free to commit > their own PRs, IMO. > > Cheers, Fabian > > [1] > http://mail-archives.apache.org/mod_mbox/flink-dev/201509.mbox/%3CCAAdrtT0RLgqJgJVrK1zH_e_7VW1k8VTv4ChnumTRh7fLGA_zmw%40mail.gmail.com%3E > > > > 2015-10-17 13:00 GMT+02:00 Alexander Alexandrov < > alexander.s.alexand...@gmail.com>: > >> Also, regarding the "Each PR should be backed by a JIRA" rule - please >> don't make this strict and consider the relative effort to opening a JIRA >> as opposed to just opening a PR for small PRs that fix something obvious. >> >> For example, two days ago I opened two PRs while investigating a reported >> JIRA bug (FLINK-2858 <https://issues.apache.org/jira/browse/FLINK-2858>): >> >> https://github.com/apache/flink/pull/1259 >> https://github.com/apache/flink/pull/1260 >> >> 1259 removes obsolete references to the (now removed 'flink-spargel' code >> from the POM), while 1260 updates a paragraph of the "How to Build" >> documentation and fixes some broken href anchors. >> >> Just my two cents here, but fixes (not only hotfixes) that >> >> * resolve minor and obvious issues with the existing code or the >> documentation, >> * can be followed by everybody just by looking at the diff >> >> should be just cherry-picked (and if needed amended) by a committer >> without >> too much unnecessary discussion and excluded from the "shepherding >> process". >> >> >> 2015-10-17 12:32 GMT+02:00 Alexander Alexandrov < >> alexander.s.alexand...@gmail.com>: >> >> > 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 >> >> >> > > >>>> >> >> >> > > >>> >> >> >> > > >>> >> >> >> > > >> >> >> >> > > > >> >> >> > > >> >> >> > > >> >> >> > >> >> >> >> >> > >> >> > >> >> >> > >> > >> > >
