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