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