Sound good to me. And I agree; I cannot comment on my own work either ;)
On 10/26/2015 05:01 PM, Fabian Hueske 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>: > >> 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 >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>>> >>> >> >
signature.asc
Description: OpenPGP digital signature
