In my experience, the template is helpful. Especially for the people just joined the community and give their first PR. I don't know how many people have read the contributor guide entirely before they commit their first PR, but I should admit that I did not read it word by word for the first time, since not all of the items related to my work. However, the template forces me to check the basic rules and guidelines of the community. Another benefit I can think of is to remind people who touch the code path they aren't familiar with. If that needs a special test flow, the template forces them to follow it.
Best, Yangze Guo On Fri, Feb 21, 2020 at 2:39 PM Yang Wang <danrtsey...@gmail.com> wrote: > > I second xintong's suggestion. When i open a PR, i also check the item list > in the template. It help to > know whether i should test the PR in a real cluster(Yarn/K8s/Mesos). Or i > should be more careful > when touching the per-record code paths.If we have some dependencies > changes, i will need to check > the generated jar as expected. > > > Best, > Yang > > Xintong Song <tonysong...@gmail.com> 于2020年2月20日周四 上午10:33写道： > > > Thanks for the feedbacks, Chesnay and Till. And thanks for the pointer, > > Congxian. > > > > I don't know how often committers and reviewers checks and benefits from > > the PR description. From your feedbacks and the number of responses to this > > discussion, it's probably not often. > > > > However, as a contributor and speaking only for myself, I actually find the > > PR template very helpful. I use it as a checking list for opening a PR. > > Filling in the template forces me to revisit the important things, e.g., > > have I added enough test cases to cover the all the important changes, does > > this change need to be validated with a real deployment (if it touches the > > deployment and recovery). An experienced developer might be able to check > > these things without such a checking list, but there might be more primary > > developers that can benefit from it. > > > > > > Therefore, if we agree that PR template is less useful for reviewers, I > > would like to propose to reposition it as a contributor checking list. The > > following are some examples of how the existing items might be > > repositioned. > > > > > > - The runtime per-record code paths (performance sensitive): (yes / no / > > don't know). If yes, please check the following items. > > > > - Is there a good reason to do that? > > - Is there an alternative non pre-record approach? > > > > - Is Java stream or Optional used in the per-recode code path? (Those > > should be avoid according to the code style and quality guide[1]) > > > > - Do we know the exact impact on performance? (Maybe point to the > > performance benchmarks) > > > > > > - Anything that affects deployment or recovery: JobManager (and its > > components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (yes / no / > > don't know). If yes, please check the following items. > > > > - Has this PR been validated with a real deployment? > > > > - Has this PR been validated with the failover scenarios? > > > > - Does this PR requires any specific version or configuration of an > > external system? E.g., Kubernetes/Yarn/Mesos/ZooKeeper APIs not supported > > by all the versions that Flink claims to be compatible with. > > > > > > WDYT? > > > > > > Thank you~ > > > > Xintong Song > > > > > > [1]https://flink.apache.org/contributing/code-style-and-quality-java.html > > > > On Wed, Feb 19, 2020 at 9:24 PM Till Rohrmann <trohrm...@apache.org> > > wrote: > > > > > I actually wanted to second Chesnay but apparently my impression is a bit > > > wrong. Out of the last 10 closed PRs (admittedly a small sample size) > > only > > > 2 did not fill out the template. I did not check for correctness though. > > > > > > Assuming that people use the template, I believe it is a good idea to > > > update it. One thing to consider is whether we wanna keep the S3 item or > > > want to generalize it. I think there was some reason why we explicitly > > > added it to the template but I cannot really remember. > > > > > > Cheers, > > > Till > > > > > > On Mon, Feb 17, 2020 at 3:02 PM Congxian Qiu <qcx978132...@gmail.com> > > > wrote: > > > > > > > JFYI, there is an issue[1] which I think is related to this thread > > > > [1] https://issues.apache.org/jira/browse/FLINK-15977 > > > > > > > > Best, > > > > Congxian > > > > > > > > > > > > Chesnay Schepler <ches...@apache.org> 于2020年2月17日周一 下午9:08写道： > > > > > > > > > I think it should just be removed since 99% of pull requests ignore > > it > > > > > anyway. > > > > > > > > > > On 17/02/2020 13:31, Xintong Song wrote: > > > > > > Hi all, > > > > > > > > > > > > It seems our PR description template is a bit outdated, and I would > > > > like > > > > > to > > > > > > propose updating it. > > > > > > > > > > > > I was working on a Kubernetes related PR, and realized that our PR > > > > > > description does not mention the new Kubernetes integration > > > questioning > > > > > > about deployment related changes. Currently is is as follows: > > > > > > > > > > > >> Anything that affects deployment or recovery: JobManager (and its > > > > > >> components), Checkpointing, Yarn/Mesos, ZooKeeper: > > > > > >> > > > > > > In addition to outdated contents, there might be other stuff that > > we > > > > want > > > > > > to add to the template. For example, I would suggest add a question > > > > about > > > > > > whether there are any memory allocation introduced by the PR, so we > > > > > review > > > > > > them carefully and avoid problems due to un-accounted memory > > > > allocations > > > > > > like FLINK-15981. (To be fair, for FLINK-15981 the memory > > allocation > > > > was > > > > > > introduced before we start to account for everything memory usage, > > > but > > > > > > noticing such memory allocations early should help us prevent > > similar > > > > > > problems in the future). > > > > > > > > > > > > Therefore, I'd also like to collect ideas on how do you think the > > > > > template > > > > > > should be updated in this discussion thread. > > > > > > > > > > > > Looking forward to your feedback~! > > > > > > > > > > > > Thank you~ > > > > > > > > > > > > Xintong Song > > > > > > > > > > > > > > > > > > > > > > > > >
