Hi all! Thank you for the interest and lively discussion.
I think we are starting to mix different things into one discussion, so I would like to close this thread and start three separate dedicated threads: (1) Contributing Guide and Pull Request Template (2) Review Process (steps, questions, checklists only, no tools like bots/labels/templates) (3) Review Tools (whether to drive the review process with labels, comments/templates, bots, etc.) Will post these threads asap. Best, Stephan On Thu, Sep 20, 2018 at 3:49 PM, 陈梓立 <wander4...@gmail.com> wrote: > Hi Fabian, > > I see a bot "project-bot" active on pull requests. It is some progress of > this thread? > > Best, > tison. > > > Thomas Weise <t...@apache.org> 于2018年9月19日周三 下午10:02写道： > > > Follow-up regarding the PR template that pops up when opening a PR: > > > > I think what we have now is a fairly big blob of text that jumps up a bit > > unexpectedly for a first time contributor and is also cumbersome to deal > > with in the small PR description window. Perhaps we can improve it a bit: > > > > * Instead of putting all that text into the description, add it to > > website/wiki and just have a pointer in the PR, asking the contributor to > > review the guidelines before opening a PR. > > * If the questions further down can be made relevant to the context of > the > > contribution, that would probably help both the contributor and the > > reviewer. For example, the questions would be different for a > documentation > > change, connector change or work deep in core. Not sure if that can be > > automated, but if moved to a separate page, it could be structured > better. > > > > Thanks, > > Thomas > > > > > > > > > > > > > > On Tue, Sep 18, 2018 at 8:13 AM 陈梓立 <wander4...@gmail.com> wrote: > > > > > Put some good cases here might be helpful. > > > > > > See how a contribution of runtime module be proposed, discussed, > > > implemented and merged from https://github.com/apache/flink/pull/5931 > > to > > > https://github.com/apache/flink/pull/6132. > > > > > > 1. #5931 fix a bug, but remains points could be improved. Here > sihuazhou > > > and shuai-xu share their considerations and require review(of the > > proposal) > > > by Stephan, Till and Gary, our committers. > > > 2. After discussion, all people involved reach a consensus. So the rest > > > work is to implement it. > > > 3. sihuazhou gives out an implementation #6132, Till reviews it and > find > > it > > > is somewhat out of the "architectural" aspect, so suggests > > > implementation-level changes. > > > 4. Addressing those implementation-level comments, the PR gets merged. > > > > > > I think this is quite a good example how we think our review process > > should > > > go. > > > > > > Best, > > > tison. > > > > > > > > > 陈梓立 <wander4...@gmail.com> 于2018年9月18日周二 下午10:53写道： > > > > > > > Maybe a little rearrange to the process would help. > > > > > > > > (1). Does the contributor describe itself well? > > > > (1.1) By whom this contribution should be given attention. This > often > > > > shows by its title, "[FLINK-XXX] [module]", the module part infer. > > > > (1.2) What the purpose of this contribution is. Done by the PR > > > template. > > > > Even on JIRA an issue should cover these points. > > > > > > > > (2). Is there consensus on the contribution? > > > > This follows (1), because we need to clear what the purpose of the > > > > contribution first. At this stage reviewers could cc to module > > maintainer > > > > as a supplement to (1.1). Also reviewers might ask the contributor to > > > > clarify his purpose to sharp(1.2) > > > > > > > > (3). Is the implement architectural and fit code style? > > > > This follows (2). And only after a consensus we talk about concrete > > > > implement, which prevent spend time and put effort in vain. > > > > > > > > In addition, ideally a "+1" comment or approval means the purpose of > > > > contribution is supported by the reviewer and implement(if there is) > > > > quality is fine, so the reviewer vote for a consensus. > > > > > > > > Best, > > > > tison. > > > > > > > > > > > > Stephan Ewen <se...@apache.org> 于2018年9月18日周二 下午6:44写道： > > > > > > > >> On the template discussion, some thoughts > > > >> > > > >> *PR Template* > > > >> > > > >> I think the PR template went well. We can rethink the "checklist" at > > the > > > >> bottom, but all other parts turned out helpful in my opinion. > > > >> > > > >> With the amount of contributions, it helps to ask the contributor to > > > take > > > >> a > > > >> little more work in order for the reviewer to be more efficient. > > > >> I would suggest to keep that mindset: Whenever we find a way that > the > > > >> contributor can prepare stuff in such a way that reviews become > > > >> more efficient, we should do that. In my experience, most > contributors > > > are > > > >> willing to put in some extra minutes if it helps that their > > > >> PR gets merged faster. > > > >> > > > >> *Review Template* > > > >> > > > >> I think it would be helpful to have this checklist. It does not > matter > > > in > > > >> which form, be that as a text template, be that as labels. > > > >> > > > >> The most important thing is to make explicit which questions have > been > > > >> answered in the review. > > > >> Currently there is a lot of "+1" on pull requests which means "code > > > >> quality > > > >> is fine", but all other questions are unanswered. > > > >> The contributors then rightfully wonder why this does not get > merged. > > > >> > > > >> > > > >> > > > >> On Tue, Sep 18, 2018 at 7:26 AM, 陈梓立 <wander4...@gmail.com> wrote: > > > >> > > > >> > Hi all interested, > > > >> > > > > >> > Within the document there is a heated discussion about how the PR > > > >> > template/review template should be. > > > >> > > > > >> > Here share my opinion: > > > >> > > > > >> > 1. For the review template, actually we don't need comment a > review > > > >> > template at all. GitHub has a tag system and only committer could > > add > > > >> tags, > > > >> > which we can make use of it. That is, tagging this PR is > > > >> > waiting-for-proposal-approved, waiting-for-code-review, > > > >> > waiting-for-benchmark or block-by-author and so on. Asfbot could > > pick > > > >> > GitHub tag state to the corresponding JIRA and we always regard > JIRA > > > as > > > >> the > > > >> > main discussion borad. > > > >> > > > > >> > 2. For the PR template, the greeting message is redundant. Just > > > >> emphasize a > > > >> > JIRA associated is important and how to format the title is > enough. > > > >> > Besides, the "Does this pull request potentially affect one of the > > > >> > following parts" part and "Documentation" should be coved from > "What > > > is > > > >> the > > > >> > purpose of the change" and "Brief change log". These two parts, > > users > > > >> > always answer no and would be aware if they really make changes on > > it. > > > >> As > > > >> > example, even pull request requires document, its owner might no > add > > > it > > > >> at > > > >> > first. The PR template is a guide but not which one have to learn. > > > >> > > > > >> > To sum up, (1) take advantage of GitHub's tag system to tag review > > > >> progress > > > >> > (2) make the template more concise to avoid burden mature > > contributors > > > >> and > > > >> > force new comer to learn too much. > > > >> > > > > >> > Best, > > > >> > tison. > > > >> > > > > >> > > > > >> > Rong Rong <walter...@gmail.com> 于2018年9月18日周二 上午7:05写道： > > > >> > > > > >> > > Thanks for putting the review contribution doc together, > Stephan! > > > This > > > >> > will > > > >> > > definitely help the community to make the review process better. > > > >> > > > > > >> > > From my experience this will benefit on both contributors and > > > >> reviewers > > > >> > > side! Thus +1 for putting into practice as well. > > > >> > > > > > >> > > -- > > > >> > > Rong > > > >> > > > > > >> > > On Mon, Sep 17, 2018 at 10:18 AM Stephan Ewen <se...@apache.org > > > > > >> wrote: > > > >> > > > > > >> > > > Hi! > > > >> > > > > > > >> > > > Thanks you for the encouraging feedback so far. > > > >> > > > > > > >> > > > The overall goal is definitely to make the contribution > process > > > >> better > > > >> > > and > > > >> > > > get fewer pull requests that are disregarded. > > > >> > > > > > > >> > > > There are various reasons for the disregarded pull requests, > one > > > >> being > > > >> > > that > > > >> > > > fewer committers really participate in reviews beyond > > > >> > > > the component they are currently very involved with. This is a > > > >> separate > > > >> > > > issue and I am thinking on how to encourage more > > > >> > > > activity there. > > > >> > > > > > > >> > > > The other reason I was lack of structure and lack of decision > > > >> making, > > > >> > > which > > > >> > > > is what I am first trying to fix here. > > > >> > > > A follow-up to this will definitely be to improve the > > contribution > > > >> > guide > > > >> > > as > > > >> > > > well. > > > >> > > > > > > >> > > > Best, > > > >> > > > Stephan > > > >> > > > > > > >> > > > > > > >> > > > On Mon, Sep 17, 2018 at 12:05 PM, Zhijiang(wangzhijiang999) < > > > >> > > > wangzhijiang...@aliyun.com.invalid> wrote: > > > >> > > > > > > >> > > > > From my personal experience as a contributor for three > years, > > I > > > >> feel > > > >> > > > > better experience in contirbuting or reviewing than before, > > > >> although > > > >> > we > > > >> > > > > still have some points for further progress. > > > >> > > > > > > > >> > > > > I reviewed the proposal doc, and it gives very constructive > > and > > > >> > > > meaningful > > > >> > > > > guides which could help both contributor and reviewer. I > agree > > > >> with > > > >> > the > > > >> > > > > bove suggestions and wish they can be praticed well! > > > >> > > > > > > > >> > > > > Best, > > > >> > > > > Zhijiang > > > >> > > > > > > > ------------------------------------------------------------------ > > > >> > > > > 发件人：Till Rohrmann <trohrm...@apache.org> > > > >> > > > > 发送时间：2018年9月17日(星期一) 16:27 > > > >> > > > > 收件人：dev <dev@flink.apache.org> > > > >> > > > > 主 题：Re: [PROPOSAL] [community] A more structured approach to > > > >> reviews > > > >> > > and > > > >> > > > > contributions > > > >> > > > > > > > >> > > > > Thanks for writing this up Stephan. I like the steps and > hope > > > >> that it > > > >> > > > will > > > >> > > > > help the community to make the review process better. Thus, > +1 > > > for > > > >> > > > putting > > > >> > > > > your proposal to practice. > > > >> > > > > > > > >> > > > > Cheers, > > > >> > > > > Till > > > >> > > > > > > > >> > > > > On Mon, Sep 17, 2018 at 10:00 AM Stephan Ewen < > > se...@apache.org > > > > > > > >> > > wrote: > > > >> > > > > > > > >> > > > > > Hi Flink community members! > > > >> > > > > > > > > >> > > > > > As many of you will have noticed, the Flink project > activity > > > has > > > >> > gone > > > >> > > > up > > > >> > > > > > again quite a bit. > > > >> > > > > > There are many more contributions, which is an absolutely > > > great > > > >> > thing > > > >> > > > to > > > >> > > > > > have :-) > > > >> > > > > > > > > >> > > > > > However, we see a continuously growing backlog of pull > > > requests > > > >> and > > > >> > > > JIRA > > > >> > > > > > issues. > > > >> > > > > > To make sure the community will be able to handle the > > > increased > > > >> > > > volume, I > > > >> > > > > > think we need to revisit some > > > >> > > > > > approaches and processes. I believe there are a few > > > >> opportunities > > > >> > to > > > >> > > > > > structure things a bit better, which > > > >> > > > > > should help to scale the development. > > > >> > > > > > > > > >> > > > > > The first thing I would like to bring up are *Pull Request > > > >> > Reviews*. > > > >> > > > Even > > > >> > > > > > though more community members being > > > >> > > > > > active in reviews (which is a really great thing!) the > Pull > > > >> Request > > > >> > > > > backlog > > > >> > > > > > is increasing quite a bit. > > > >> > > > > > > > > >> > > > > > Why are pull requests still not merged faster? Looking at > > the > > > >> > > reviews, > > > >> > > > > one > > > >> > > > > > thing I noticed is that most reviews deal > > > >> > > > > > immediately with detailed code issues, and leave out most > of > > > the > > > >> > core > > > >> > > > > > questions that need to be answered > > > >> > > > > > before a Pull Request can be merged, like "is this a > desired > > > >> > > feature?" > > > >> > > > or > > > >> > > > > > "does this align well with other developments?". > > > >> > > > > > I think that we even make things slightly worse that way: > > From > > > >> my > > > >> > > > > personal > > > >> > > > > > experience, I have often thought "oh, this > > > >> > > > > > PR has a review already" and rather looked at another PR, > > only > > > >> to > > > >> > > find > > > >> > > > > > later that the first review did never decide whether > > > >> > > > > > this PR is actually a good fit for Flink. > > > >> > > > > > > > > >> > > > > > There has never been a proper documentation of how to > answer > > > >> these > > > >> > > > > > questions, what to evaluate in reviews, > > > >> > > > > > guidelines for how to evaluate pull requests, other than > > code > > > >> > > quality. > > > >> > > > I > > > >> > > > > > suspect that this is why so many reviewers > > > >> > > > > > do not address the "is this a good contribution" > questions, > > > >> making > > > >> > > pull > > > >> > > > > > requests linger until another committers joins > > > >> > > > > > the review. > > > >> > > > > > > > > >> > > > > > Below is an idea for a guide *"How to Review > > Contributions"*. > > > It > > > >> > > > outlines > > > >> > > > > > five core aspects to be checked in every > > > >> > > > > > pull request, and suggests a priority for clarifying > those. > > > The > > > >> > idea > > > >> > > is > > > >> > > > > > that this helps us to better structure reviews, and > > > >> > > > > > to make each reviewer aware what we look for in a review > and > > > >> where > > > >> > to > > > >> > > > > best > > > >> > > > > > bring in their help. > > > >> > > > > > > > > >> > > > > > Looking forward to comments! > > > >> > > > > > > > > >> > > > > > Best, > > > >> > > > > > Stephan > > > >> > > > > > > > > >> > > > > > ==================================== > > > >> > > > > > > > > >> > > > > > The draft is in this Google Doc. Please add small textual > > > >> comments > > > >> > to > > > >> > > > the > > > >> > > > > > doc, and bigger principle discussions as replies to this > > mail. > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > https://docs.google.com/document/d/1yaX2b9LNh-6LxrAmE23U3D2c > > > >> > > > > RbocGlGKCYnvJd9lVhk/edit?usp=sharing > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > *How to Review > > Contributions------------------------------This > > > >> > guide > > > >> > > is > > > >> > > > > for > > > >> > > > > > all committers and contributors that want to help with > > > reviewing > > > >> > > > > > contributions. Thank you for your effort - good reviews > are > > > one > > > >> the > > > >> > > > most > > > >> > > > > > important and crucial parts of an open source project. > This > > > >> guide > > > >> > > > should > > > >> > > > > > help the community to make reviews such that: - > Contributors > > > >> have a > > > >> > > > good > > > >> > > > > > contribution experience- Reviews are structured and check > > all > > > >> > > important > > > >> > > > > > aspects of a contribution- Make sure we keep a high code > > > >> quality in > > > >> > > > > Flink- > > > >> > > > > > We avoid situations where contributors and reviewers > spend a > > > >> lot of > > > >> > > > time > > > >> > > > > to > > > >> > > > > > refine a contribution that gets rejected laterReview > > > >> ChecklistEvery > > > >> > > > > review > > > >> > > > > > needs to check the following five aspects. We encourage to > > > check > > > >> > > these > > > >> > > > > > aspects in order, to avoid spending time on detailed code > > > >> quality > > > >> > > > reviews > > > >> > > > > > when there is not yet consensus that a feature or change > > > should > > > >> be > > > >> > > > > actually > > > >> > > > > > be added.(1) Is there consensus whether the change of > > feature > > > >> > should > > > >> > > go > > > >> > > > > > into to Flink?For bug fixes, this needs to be checked only > > in > > > >> case > > > >> > it > > > >> > > > > > requires bigger changes or might break existing programs > and > > > >> > > > > > setups.Ideally, this question is already answered from a > > JIRA > > > >> issue > > > >> > > or > > > >> > > > > the > > > >> > > > > > dev-list discussion, except in cases of bug fixes and > small > > > >> > > lightweight > > > >> > > > > > additions/extensions. In that case, this question can be > > > >> > immediately > > > >> > > > > marked > > > >> > > > > > as resolved. For pull requests that are created without > > prior > > > >> > > > consensus, > > > >> > > > > > this question needs to be answered as part of the > review.The > > > >> > decision > > > >> > > > > > whether the change should go into Flink needs to take the > > > >> following > > > >> > > > > aspects > > > >> > > > > > into consideration: - Does the contribution alter the > > behavior > > > >> of > > > >> > > > > features > > > >> > > > > > or components in a way that it may break previous users’ > > > >> programs > > > >> > and > > > >> > > > > > setups? If yes, there needs to be a discussion and > agreement > > > >> that > > > >> > > this > > > >> > > > > > change is desirable. - Does the contribution conceptually > > fit > > > >> well > > > >> > > into > > > >> > > > > > Flink? Is it too much of special case such that it makes > > > things > > > >> > more > > > >> > > > > > complicated for the common case, or bloats the > abstractions > > / > > > >> > APIs? - > > > >> > > > > Does > > > >> > > > > > the feature fit well into Flink’s architecture? Will it > > scale > > > >> and > > > >> > > keep > > > >> > > > > > Flink flexible for the future, or will the feature > restrict > > > >> Flink > > > >> > in > > > >> > > > the > > > >> > > > > > future? - Is the feature a significant new addition > (rather > > > >> than an > > > >> > > > > > improvement to an existing part)? If yes, will the Flink > > > >> community > > > >> > > > commit > > > >> > > > > > to maintaining this feature? - Does the feature produce > > added > > > >> value > > > >> > > for > > > >> > > > > > Flink users or developers? Or does it introduce risk of > > > >> regression > > > >> > > > > without > > > >> > > > > > adding relevant user or developer benefit?All of these > > > questions > > > >> > > should > > > >> > > > > be > > > >> > > > > > answerable from the description/discussion in JIRA and > Pull > > > >> > Request, > > > >> > > > > > without looking at the code.(2) Does the contribution need > > > >> > attention > > > >> > > > from > > > >> > > > > > some specific committers and is there time commitment from > > > these > > > >> > > > > > committers?Some changes require attention and approval > from > > > >> > specific > > > >> > > > > > committers. For example, changes in parts that are either > > very > > > >> > > > > performance > > > >> > > > > > sensitive, or have a critical impact on distributed > > > coordination > > > >> > and > > > >> > > > > fault > > > >> > > > > > tolerance need input by a committer that is deeply > familiar > > > with > > > >> > the > > > >> > > > > > component.As a rule of thumb, this is the case when the > Pull > > > >> > Request > > > >> > > > > > description answers one of the questions in the template > > > section > > > >> > > “Does > > > >> > > > > this > > > >> > > > > > pull request potentially affect one of the following > parts” > > > with > > > >> > > > > ‘yes’.This > > > >> > > > > > question can be answered with - Does not need specific > > > >> attention- > > > >> > > Needs > > > >> > > > > > specific attention for X (X can be for example > > checkpointing, > > > >> > > > jobmanager, > > > >> > > > > > etc.).- Has specific attention for X by @commiterA, > > > >> @contributorBIf > > > >> > > the > > > >> > > > > > pull request needs specific attention, one of the tagged > > > >> > > > > > committers/contributors should give the final approval.(3) > > Is > > > >> the > > > >> > > > > > contribution described well?Check whether the contribution > > is > > > >> > > > > sufficiently > > > >> > > > > > well described to support a good review. Trivial changes > and > > > >> fixes > > > >> > do > > > >> > > > not > > > >> > > > > > need a long description. Any pull request that changes > > > >> > functionality > > > >> > > or > > > >> > > > > > behavior needs to describe the big picture of these > changes, > > > so > > > >> > that > > > >> > > > > > reviews know what to look for (and don’t have to dig > through > > > the > > > >> > code > > > >> > > > to > > > >> > > > > > hopefully understand what the change does).Changes that > > > require > > > >> > > longer > > > >> > > > > > descriptions are ideally based on a prior design > discussion > > in > > > >> the > > > >> > > > > mailing > > > >> > > > > > list or in JIRA and can simply link to there or copy the > > > >> > description > > > >> > > > from > > > >> > > > > > there.(4) Does the implementation follow the right overall > > > >> > > > > > approach/architecture?Is this the best approach to > implement > > > the > > > >> > fix > > > >> > > or > > > >> > > > > > feature, or are there other approaches that would be > easier, > > > >> more > > > >> > > > robust, > > > >> > > > > > or more maintainable?This question should be answerable > from > > > the > > > >> > Pull > > > >> > > > > > Request description (or the linked JIRA) as much as > > > possible.We > > > >> > > > recommend > > > >> > > > > > to check this before diving into the details of commenting > > on > > > >> > > > individual > > > >> > > > > > parts of the change.(5) Is the overall code quality good, > > > >> meeting > > > >> > > > > standard > > > >> > > > > > we want to maintain in Flink?This is the detailed code > > review > > > of > > > >> > the > > > >> > > > > actual > > > >> > > > > > changes, covering: - Are the changes doing what is > described > > > in > > > >> the > > > >> > > > > design > > > >> > > > > > document or PR description?- Does the code follow the > right > > > >> > software > > > >> > > > > > engineering practices? It the code correct, robust, > > > >> maintainable, > > > >> > > > > > testable?- Are the change performance aware, when > changing a > > > >> > > > performance > > > >> > > > > > sensitive part?- Are the changes sufficiently covered by > > > tests?- > > > >> > Are > > > >> > > > the > > > >> > > > > > tests executing fast?- Does the code format follow Flink’s > > > >> > checkstyle > > > >> > > > > > pattern?- Does the code avoid to introduce additional > > compiler > > > >> > > > > > warnings?Some code style guidelines can be found in the > > [Flink > > > >> Code > > > >> > > > Style > > > >> > > > > > Page]( > > > https://flink.apache.org/contribute-code.html#code-style > > > >> > > > > > <https://flink.apache.org/contribute-code.html#code-style > > > >)Pull > > > >> > > > Request > > > >> > > > > > Review TemplateAdd the following checklist to the pull > > request > > > >> > > review, > > > >> > > > > > checking the boxes as the questions are answered: - [ ] > > > >> Consensus > > > >> > > that > > > >> > > > > the > > > >> > > > > > contribution should go into to Flink - [ ] Does not need > > > >> specific > > > >> > > > > > attention | Needs specific attention for X | Has attention > > > for X > > > >> > by Y > > > >> > > > - > > > >> > > > > [ > > > >> > > > > > ] Contribution description - [ ] Architectural approach > - > > [ > > > ] > > > >> > > Overall > > > >> > > > > > code quality* > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > > > > > >
