+1 for that one. The good news is all Flink committers have been very discipline about reviews :)
On Monday, October 26, 2015, Fabian Hueske <fhue...@gmail.com> wrote: > Hi Matthias, > > those a good points. I did not really think about the different roles > (technical, meta-role). > My reasoning was that the shepherd should be able to give feedback on the > PR in order to move it forward. This does not work so well if the shepherd > is also the author of the PR (at least I am not so good at commenting on my > own work ;-)). > > However, you and Henry are of course right. Every committer can commit by > her/himself. > > How about we keep it as follows: > Committers can of course push hot fixes and minor changes directly. > Everything larger should go through a PR. If nobody signs up to shepherd > the PR, the author can drive it forward her/himself. I don't think it is > necessary that a committer explicitly sign-up as a shepherd of her/his own > PR. > > Cheers, Fabian > > > 2015-10-24 20:48 GMT+02:00 Henry Saputra <henry.sapu...@gmail.com > <javascript:;>>: > > > Yes, as committer we trust you to do the right thing when committing the > > code. > > If a committer believe he/ she needs review,especially with large > > patch, then he/ she should send PR for review. > > > > > > - Henry > > > > On Sat, Oct 24, 2015 at 3:53 AM, Matthias J. Sax <mj...@apache.org > <javascript:;>> wrote: > > > Great work! > > > > > > What puzzles me a little bit, is the shepherding of PR from committers. > > > Why should it be a different committer? > > > > > > 1) As a committer, you don't even need to open a PR for small code > > > changes at all (especially, if the committer is the most experience one > > > regard a certain component/library). Just open a JIRA, fix it, and > > commit. > > > > > > 2) It is clear, that if a PR is complex and maybe touches different > > > components, feedback from the most experiences committer on those > > > components should be given on the PR to ensure code quality. But the > > > role of a shepherd is a "meta" role, if a understand the guideline > > > correctly, and not a technical one (-> everybody should discuss, > > > comment, accept, mark to get merged, and merge PRs). So why do we need > a > > > different shepherd there? I think, committers can drive their own PRs > by > > > themselves. > > > > > > > > > -Matthias > > > > > > > > > On 10/23/2015 06:00 PM, Fabian Hueske wrote: > > >> 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 > <javascript:;>>: > > >> > > >>> 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 <javascript:;>>: > > >>> > > >>>> 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 <javascript:;>>: > > >>>> > > >>>>> 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 > <javascript:;>>: > > >>>>> > > >>>>>> 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 > <javascript:;>>: > > >>>>>> > > >>>>>>> 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 <javascript:;>>: > > >>>>>>> > > >>>>>>>>> > > >>>>>>>>> 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 <javascript:;> > > >>>>>>>>> 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 > <javascript:;>> > > >>>> 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 <javascript:;>>: > > >>>>>>>>>>> > > >>>>>>>>>>>> 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 <javascript:;>> > > >>>>>>>>>> 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 > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>> > > >>>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>> > > >>>>> > > >>>>> > > >>>> > > >>> > > >>> > > >> > > > > > >
