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 <tzuli...@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 (u...@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 <se...@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)**
signature.asc
Description: This is a digitally signed message part.
