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