(DEPRECATED) Apache Flink Mailing List archive.

[Discuss] Update the pull request description template.

Classic

List

Threaded

9 messages Options

Xintong Song

[Discuss] Update the pull request description template.

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

Chesnay Schepler-3

Re: [Discuss] Update the pull request description template.

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
>

Congxian Qiu

Re: [Discuss] Update the pull request description template.

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 <[hidden email]> 于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
> >
>
>

Till Rohrmann

Re: [Discuss] Update the pull request description template.

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 <[hidden email]> 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 <[hidden email]> 于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
> > >
> >
> >
>

Xintong Song

Re: [Discuss] Update the pull request description template.

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 <[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]> 于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
> > > >
> > >
> > >
> >
>

Yang Wang

Re: [Discuss] Update the pull request description template.

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 <[hidden email]> 于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 <[hidden email]>
> 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 <[hidden email]>
> > 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 <[hidden email]> 于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
> > > > >
> > > >
> > > >
> > >
> >
>

Yangze Guo

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 <[hidden email]> 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 <[hidden email]> 于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 <[hidden email]>
> > 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 <[hidden email]>
> > > 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 <[hidden email]> 于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
> > > > > >
> > > > >
> > > > >
> > > >
> > >
> >

Zhijiang(wangzhijiang999)

Re: [Discuss] Update the pull request description template.

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 <[hidden email]>
Send Time:2020 Feb. 21 (Fri.) 16:16
To:dev <[hidden email]>
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 <[hidden email]> wrote:

tison

Re: [Discuss] Update the pull request description template.

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 <[hidden email]> 于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 <[hidden email]>
> Send Time:2020 Feb. 21 (Fri.) 16:16
> To:dev <[hidden email]>
> 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 <[hidden email]> 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 <[hidden email]> 于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 <[hidden email]>
> > > 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 <[hidden email]
> >
> > > > 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 <[hidden email]> 于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
> > > > > > >
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
>
>