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