Thanks for kicking off the discussion and opinions so far. I'd like to share two of my coins.
1. Often I check the item list when review a non-trivial pull request and I think it works for some specific topics, such as document coverage, general description and test coverage description. 2. We always have a checklist which attach later by flink-bot that has some overlaps of the item list in pull request template. It would be a good topic how we integrate these two list. Maybe we don't just merge them but properly reorder items so that attentions can be applied gradually(for example, most of pull request is unrelated to per record codepath, but for those are, it is important to emphasize). Best, tison. Zhijiang <wangzhijiang...@aliyun.com.invalid> 于2020年2月21日周五 下午11:52写道： > Thanks for launching this discussion and all the involved feedbacks! > > Since there are still many users relying on the template to raise > attentions and guide review, it sounds reasonable to update the template > based on demands. > > In my experience, I was always filling the template when submitting PRs > before. But I found a bit trouble for filling the last two sections > "Does this pull request potentially affect one of the following parts:" > and "Documentation", because I had to either remove one from "yes|no" or > highlight one > manually for every listed item. And I guess for most of PRs, the results > of these items should be "no" by default. > > If we can refactor to another description here, E.g. "Selecting the > following parts which this pull request potentially affects", and further > make every item selectable instead. > Then most of the users do not need to touch these sections by default , > which means without implication. I guess it would save some efforts. > > My above concern is tiny and might not be the key motivation of this > discussion. Just share my thoughts by this chance. :) > > Best, > Zhijiang > > > ------------------------------------------------------------------ > From:Yangze Guo <karma...@gmail.com> > Send Time:2020 Feb. 21 (Fri.) 16:16 > To:dev <dev@flink.apache.org> > Subject:Re: [Discuss] Update the pull request description template. > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
