flink-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nico Kruber <n...@data-artisans.com>
Subject Re: [DISCUSS] A more thorough Pull Request check list and template
Date Tue, 18 Jul 2017 15:35:49 GMT
I like the new template but also agree with the text being too long and would 
move the intro to the contributors guide with a link in the PR template.

Regarding the questions to fill out - I'd like the headings to be short and 
have the affected components last so that documentation is not lost (although 
being more important than this checklist), e.g.:

* Purpose of the change
* Brief change log
* Verifying the change
* Documentation
* Affected components

The verification options in the original template look a bit too large but it 
stresses what tests should be added, especially for bigger changes. Can't 
think of a way to make it shorter though.


Nico

On Tuesday, 18 July 2017 11:20:41 CEST Chesnay Schepler wrote:
> I fully agree with Fabian.
> 
> Multiple-choice questions provide little value to the reviewer, since the
> validity has to be verified in any case. While text answers have to be
> validated as well,
> they give some hint to the reviewer as to how it can be verified and
> which steps the
> contributor did to do so.
> 
> I also agree that it is too long; IMO this is really intimidating to new
> contributors to be greeted with this.
> 
> Ideally we only link to the contributors guide and ask 3 questions:
> 
>   * What is the problem?
>   * How was it fixed?
>   * How can the fix be verified?
> 
> On 18.07.2017 10:47, Fabian Hueske wrote:
> > I like the sections about purpose, change log, and verification of the
> > changes.
> > 
> > However, I think the proposed template is too much text. This is probably
> > the reason why the first attempt to establish a PR template failed.
> > I would move most of the introduction and explanations incl. examples to
> > the "Contribution Guidelines" and only pass a link.
> > IMO, the template should be rather shorter than the current one and only
> > have the link, the sections to fill out, and checkboxes.
> > 
> > I'm also not sure how much the detailed questions will help.
> > For example even if the question about changed dependencies is answered
> > with "no", the reviewer still has to check that.
> > 
> > I think the questions of the current template work differently.
> > A question "Does the PR include tests?" suggests to the contributor that
> > those should be included. Same for documentation.
> > 
> > Cheers,
> > Fabian
> > 
> > 2017-07-18 10:05 GMT+02:00 Tzu-Li (Gordon) Tai <tzulitai@apache.org>:
> >> +1, I like this a lot.
> >> With the previous template, it doesn’t really resonate with what we
> >> should
> >> care about, and therefore most of the time I think contributors just
> >> delete
> >> that template and write down something on their own.
> >> 
> >> I would also like to add: “Savepoint / checkpoint binary formats” to the
> >> potential affect scope check list.
> >> I think changes to those deserves an independent yes / no check of its
> >> own.
> >> 
> >> Cheers,
> >> Gordon
> >> 
> >> On 18 July 2017 at 3:49:42 PM, Ufuk Celebi (uce@apache.org) wrote:
> >> 
> >> I really like this and vote to change our current template.
> >> 
> >> The simple yes/no/... options are a really good idea. I would add to
> >> your email that the questions will equally help reviewers to remember
> >> to look at these things, which is just as important.
> >> 
> >> When we merge this, we should make sure to strictly follow the guide.
> >> Ideally, in the long term we can even automate some of the yes/no/...
> >> questions via a bot... but let's not get ahead of ourselves here ;-)
> >> 
> >> On Mon, Jul 17, 2017 at 6:36 PM, Stephan Ewen <sewen@apache.org> wrote:
> >>> Hi all!
> >>> 
> >>> I have reflected a bit on the pull requests and on some of the recent
> >>> changes to Flink and some of the introduced bugs / regressions that we
> >> 
> >> have
> >> 
> >>> fixed.
> >>> 
> >>> One thing that I think would have helped is to have more explicit
> >>> information about what the pull request does and how the contributor
> >> 
> >> would
> >> 
> >>> suggest to verify it. I have seen this when contributing to some other
> >>> project and really liked the approach.
> >>> 
> >>> It requires that a contributor takes a minute to reflect on what was
> >>> touched, and what would be ways to verify that the changes work
> >>> properly.
> >>> Besides being a help to the reviewer, it also makes contributors aware
> >>> of
> >>> what is important during the review process.
> >>> 
> >>> 
> >>> I suggest a new pull request template, as attached below, with a preview
> >>> here:
> >>> https://github.com/StephanEwen/incubator-flink/
> >> 
> >> blob/pr_template/.github/PULL_REQUEST_TEMPLATE.md
> >> 
> >>> Don't be scared, it looks long, but a big part is the introductory text
> >>> (only relevant for new contributors) and the examples contents for the
> >>> description.
> >> 
> >>> Filling this out for code that is in shape should be a quick thing:
> >> Remove
> >> 
> >>> the into and checklist, write a few sentences on what the PR does (one
> >>> should do that anyways) and then pick some yes/no in the classification
> >>> section.
> >>> 
> >>> Curious to hear what you think!
> >>> 
> >>> Best,
> >>> Stephan
> >>> 
> >>> 
> >>> ============================
> >>> 
> >>> Full suggested pull request template:
> >>> 
> >>> 
> >>> 
> >>> *Thank you very much for contributing to Apache Flink - we are happy
> >>> that
> >>> you want to help us improve Flink. To help the community review you
> >>> contribution in the best possible way, please go through the checklist
> >>> below, which will get the contribution into a shape in which it can be
> >> 
> >> best
> >> 
> >>> reviewed.*
> >>> 
> >>> *Please understand that we do not do this to make contributions to Flink
> >> 
> >> a
> >> 
> >>> hassle. In order to uphold a high standard of quality for code
> >>> contributions, while at the same time managing a large number of
> >>> contributions, we need contributors to prepare the contributions well,
> >> 
> >> and
> >> 
> >>> give reviewers enough contextual information for the review. Please also
> >>> understand that contributions that do not follow this guide will take
> >>> longer to review and thus typically be picked up with lower priority by
> >> 
> >> the
> >> 
> >>> community.*
> >>> 
> >>> ## Contribution Checklist
> >>> 
> >>> - Make sure that the pull request corresponds to a [JIRA issue](
> >>> https://issues.apache.org/jira/projects/FLINK/issues). Exceptions are
> >> 
> >> made
> >> 
> >>> for typos in JavaDoc or documentation files, which need no JIRA issue.
> >>> 
> >>> - Name the pull request in the form "[FLINK-1234] [component] Title of
> >>> the pull request", where *FLINK-1234* should be replaced by the actual
> >>> issue number. Skip *component* if you are unsure about which is the best
> >>> component.
> >>> Typo fixes that have no associated JIRA issue should be named following
> >>> this pattern: `[hotfix] [docs] Fix typo in event time introduction` or
> >>> `[hotfix] [javadocs] Expand JavaDoc for PuncuatedWatermarkGenerator`.
> >>> 
> >>> - Fill out the template below to describe the changes contributed by the
> >>> pull request. That will give reviewers the context they need to do the
> >>> review.
> >>> 
> >>> - Make sure that the change passes the automated tests, i.e., `mvn clean
> >>> verify`
> >>> 
> >>> - Each pull request should address only one issue, not mix up code from
> >>> multiple issues.
> >>> 
> >>> - Each commit in the pull request has a meaningful commit message
> >>> (including the JIRA id)
> >>> 
> >>> - Once all items of the checklist are addressed, remove the above text
> >>> and this checklist, leaving only the filled out template below.
> >>> 
> >>> 
> >>> **(The sections below can be removed for hotfixes of typos)**
> >>> 
> >>> ## What is the purpose of the change
> >>> 
> >>> *(For example: This pull request makes task deployment go through the
> >> 
> >> blob
> >> 
> >>> server, rather than through RPC. That way we avoid re-transferring them
> >> 
> >> on
> >> 
> >>> each deployment (during recovery).)*
> >>> 
> >>> 
> >>> ## Brief change log
> >>> 
> >>> *(for example:)*
> >>> - *The TaskInfo is stored in the blob store on job creation time as a
> >>> persistent artifact*
> >>> - *Deployments RPC transmits only the blob storage reference*
> >>> - *TaskManagers retrieve the TaskInfo from the blob cache*
> >>> 
> >>> 
> >>> ## Verifying this change
> >>> 
> >>> *(Please pick either of the following options)*
> >>> 
> >>> This change is a trivial rework / code cleanup without any test
> >>> coverage.
> >>> 
> >>> *(or)*
> >>> 
> >>> This change is already covered by existing tests, such as *(please
> >> 
> >> describe
> >> 
> >>> tests)*.
> >>> 
> >>> *(or)*
> >>> 
> >>> This change added tests and can be verified as follows:
> >>> 
> >>> *(example:)*
> >>> - *Added integration tests for end-to-end deployment with large payloads
> >>> (100MB)*
> >>> - *Extended integration test for recovery after master (JobManager)
> >>> failure*
> >>> - *Added test that validates that TaskInfo is transferred only once
> >>> across recoveries*
> >>> - *Manually verified the change by running a 4 node cluser with 2
> >>> JobManagers and 4 TaskManagers, a stateful streaming program, and
> >>> killing
> >>> one JobManager and to TaskManagers during the execution, verifying that
> >>> recovery happens correctly.*
> >>> 
> >>> ## Does this pull request potentially affect one of the following parts:
> >>> 
> >>> - Dependencies (does it add or upgrade a dependency): **(yes / no)**
> >>> - The public API, i.e., is any changed class annotated with
> >>> `@Public(Evolving)`: **(yes / no)**
> >>> - The serializers: **(yes / no / don't know)**
> >>> - The runtime per-record code paths (performance sensitive): **(yes / no
> >>> / don't know)**
> >>> - Anything that affects deployment or recovery: JobManager (and its
> >>> components), Checkpointing, Yarn/Mesos, ZooKeeper: **(yes / no / don't
> >>> know)**:
> >>> 
> >>> ## Documentation
> >>> 
> >>> - Does this pull request introduce a new feature? **(yes / no)**
> >>> - If yes, how is the feature documented? **(not applicable / docs /
> >>> JavaDocs / not documented)**


Mime
View raw message