osdir.com


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PROPOSAL] [community] A more structured approach to reviews and contributions


Hi,

I would prefer the integrated idea and could help to concrete it.

Posting a review template once a pull request created. This is regarded as
the formal process reviews to follow. Posting a checklist in form and make
it editable for maintainer.

[ ] Reach consensus
[ ] Implementation architectural
[ ] Code quality admired
( can with more check item and description if needed )

When the process go ahead, maintainer, i.e., committers SHOULD update the
checklist, and SUGGESTED to add a label to mark the stage. Ideally when
stage changed, maintainer/committer add a corresponding comment to explain
it. Of course when things are obvious, some works could be omit.

Best,
tison.


Fabian Hueske <fhueske@xxxxxxxxx> 于2018年9月19日周三 下午8:14写道:

> Hi,
>
> I'd like to suggest that we keep this thread focused on discussing
> Stephan's proposal, i.e., introducing a structured PR review process.
> Tison and Piotr raised some good points related to PR reviews that are
> definitely worth discussing, but I think we should do that on different
> threads and move forward with the original discussion.
>
> Stephan's proposal has received only positive feedback until now.
> So, I think we should go ahead and adopt the process.
>
> IMO, there are two things to be done here:
>
> * Add the proposed process to the website (trivial)
> * Implement the process in practice which is still an open question to me.
>
> Two methods have been proposed to implement the process and I see following
> dis/advantages:
>
> 1. Automatically posting the review checklist as the first comment to a PR
> and track the progress by ticking off boxes in the checklist.
>
>   + First time contributors / reviewers learn about the process from the
> comment, which reduces the chance of detailed reviews without consensus on
> the motivation and approach.
>   - Comments are not visible when looking at the PR list and cannot be used
> for filtering.
>   - Needs some kind of service that automatically comments on PRs. Service
> does not need special permissions as every GH user can comment.
>
> 2. Tracking the review status of a PR with labels.
>
>   + Labels are visible on the PR overview and can be used to filter PRs.
>   - The review process is not spelled out. Contributors and reviewers have
> to learn about the process somewhere else (link could be added to PR
> template).
>   - If we want to tick-off labels, we needs some kind of service to
> automatically assign labels. The service needs committer permissions or be
> setup by ASF Infra.
>   - We need to check if assignment/removal of labels is mirrored to a
> mailing list which is important in the ASF to track decision. This
> shouldn't be hard to figure out, but if labels are not tracked, they cannot
> be the sole solution.
>
> We can of course also do both.
> Have the review checklist posted and tracked as a comment and ask reviewers
> to add the right label when ticking a box off.
>
> What do you think?
>
> Best,
> Fabian
>
> 2018-09-19 11:24 GMT+02:00 陈梓立 <wander4096@xxxxxxxxx>:
>
> > Hi Piotr,
> >
> > I strongly agree with your idea. Since we work on a huge project, diff in
> > the PR becomes hard to review if there is too much noise.
> >
> > Just to clarify the process. Do you mean that a pull request should go
> into
> > the way below?
> >
> > Separated commits during the implement, mainly distinguish feature/bug
> fix
> > with clean up/rework.
> >
> > [FLINK-XXX] [module] future/bug fix...
> > [FLINK-XXX] [module] more on future/bug fix...
> > [hotfix] clean up/rework...
> >
> > and so on.
> >
> > And finally, when get merged, put all stuff into one commit and comments
> > close #PRID
> > so coming ones could see the detail by jump to #PRID.
> >
> > One thing to trade off is that if we merge by one commit, we cannot
> revert
> > part of it automated; if we merge by PR commits(one by one), the commit
> log
> > might mess.
> >
> > Best,
> > tison.
> >
> >
> > Piotr Nowojski <piotr@xxxxxxxxxxxxxxxxx> 于2018年9月19日周三 下午5:10写道:
> >
> > > Hi,
> > >
> > > I would like to rise one more issue. Often contributions are very heavy
> > > and difficult to review. They have one big commit that changes multiple
> > > things which is difficult to review. Instead I would be in favour of
> > > implementing a rule, that a single commit should never do more then one
> > > thing. For example never mixing refactoring/renames/clean ups/code
> > > deduplications with functional changes. If implementing something
> > requires
> > > two independent changes in two separate components, those also should
> be
> > > two independent commits. In other words, if there are two changed lines
> > in
> > > a single commit, they should interact somehow together and strictly
> > depend
> > > on one another. This has couple of important advantages:
> > >
> > > 1. Obviously faster reviews. Especially if a reviewer do not have to
> find
> > > 2 lines bug fix among 200 lines of renames/refactoring.
> > > 2. Provides good “cut out” points for reviewer. For example he can
> easily
> > > interrupt reviewing in the middle and continue later or even merge PR
> in
> > > stages.
> > > 3. Better reference “why something was done this way not the other” for
> > > the future. This is the same argument as first point, however with
> > benefit
> > > not during reviewing, but when after merging someone is trying to
> > > understand the code.
> > > 4. Commit message becomes much better place to write down reasons why
> > > something was done and what are the effects (not that this should
> replace
> > > comments/documentation, only to complement it).
> > > 5. In case of need to revert/drop some part of the contribution, we are
> > > not loosing all of it. If we have to revert some small feature, it
> would
> > be
> > > easier to keep refactoring and clean ups.
> > >
> > >
> > > Some examples of PRs that were more or less following this rule:
> > > https://github.com/apache/flink/pull/6692/commits
> > > https://github.com/apache/flink/pull/5423/commits <
> > > https://github.com/apache/flink/pull/5423/commits> (a bit extreme)
> > >
> > > If someone is not convinced I encourage to open those PRs and browse
> > > through couple of first commits (which are refactoring/clean up
> commits)
> > > one by one (GitHub has next/prev commit button). Then imagine if they
> > were
> > > squashed with some functional/performance improvement changes.
> > >
> > > Piotrek
> > >
> > > > On 18 Sep 2018, at 17:12, 陈梓立 <wander4096@xxxxxxxxx> 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.
> > > >
> > > >
> > > > 陈梓立 <wander4096@xxxxxxxxx> 于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 <sewen@xxxxxxxxxx> 于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, 陈梓立 <wander4096@xxxxxxxxx> 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 <walterddr@xxxxxxxxx> 于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 <sewen@xxxxxxxxxx>
> > > >>> 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) <
> > > >>>>>> wangzhijiang999@xxxxxxxxxx.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 <trohrmann@xxxxxxxxxx>
> > > >>>>>>> 发送时间:2018年9月17日(星期一) 16:27
> > > >>>>>>> 收件人:dev <dev@xxxxxxxxxxxxxxxx>
> > > >>>>>>> 主 题: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 <
> sewen@xxxxxxxxxx>
> > > >>>>> 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*
> > > >>>>>>>>
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>
> > >
> > >
> >
>