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