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