> If the author haven't provided the context, any committer who want to add the release/* label should left a comment about why the PR should be cherry-pick.
Totally agree. The committer needs to add at least one comment for the reason if he wants to add the release labels. I think we could add this to the committer guideline if we reach this consensus. Thanks, Zike Yang On Tue, Feb 28, 2023 at 3:19 PM PengHui Li <peng...@apache.org> wrote: > > I found another example https://github.com/apache/pulsar/pull/19425 > Which has cherry-picked and then reverted > > If the PR's author knows the PR should be cherry-pick to some branches, > it's better to contains this part in the PR description and explain why the > fix > should be cherry-picked. The reviewer should also review whether it is > worth cherry-picking and then updating the labels. > > If the author haven't provided the context, any committer who want to add > the release/* label should left a comment about why the PR should be > cherry-pick. > > I think it will helped the release manager to understand should or > shouldn't > to cherry-pick the PRs. > > We can also update the PR template > > Thanks, > Penghui > > > On Tue, Feb 28, 2023 at 1:36 PM Dave Fisher <wave4d...@comcast.net> wrote: > > > > > > > Sent from my iPhone > > > > > On Feb 27, 2023, at 9:28 PM, tison <wander4...@gmail.com> wrote: > > > > > > > > >> > > >> Yes, but.. > > > > > > For this case, I agree that the RM trust Jason who tagged the PR needs to > > > be picked to 2.10. In this case Jason was supposed to do the check. > > > > > >> guidance for release managers to evaluate PR cherry-pick labels > > carefully > > > > > > For the current workflow, the RM can trust the labels since only > > committers > > > can label a PR. > > > > The RM should still verify what they are trusting. People make mistakes. > > > > > > We may add some guidelines in both label strategy and release process on > > > the Contribution Guide to talk about breaking changes. Generally: > > > > > > * Default config value. > > > * Public API breaking. > > > * Dependency upgrading (may or may not a breaking change) > > > * Other user-visible behavior changes (in the issued PR, it changes > > whether > > > an offload data is deleted on topic deleted) > > > > > > There's no automation/tests to cover all the cases so we still rely on > > > proper reviews and tags. > > > > > > Best, > > > tison. > > > > > > > > > Dave Fisher <wave4d...@comcast.net> 于2023年2月28日周二 13:21写道： > > > > > >> > > >> > > >> Sent from my iPhone > > >> > > >>>> On Feb 27, 2023, at 9:08 PM, tison <wander4...@gmail.com> wrote: > > >>> > > >>> > > >>>> > > >>>> The release manager is unable to review all PRs before releasing it. > > >>> > > >>> At least the RM is responsible for PRs cherry-picked he/she made. As we > > >>> take compatibility in a high priority, if it's unclear a fix (patch) > > >>> without breaking changes, the RM can ask for confirmation. > > >> > > >> Is there guidance for release managers to evaluate PR cherry-pick labels > > >> carefully (how to confirm)? > > >> > > >> Best, > > >> Dave > > >>> > > >>> Best, > > >>> tison. > > >>> > > >>> > > >>> PengHui Li <peng...@apache.org> 于2023年2月28日周二 12:45写道： > > >>> > > >>>> Hi enrico, > > >>>> > > >>>> +1 for your point. > > >>>> > > >>>> Do you know the details of the breaking change? > > >>>> I can't find any discussions under the mailing list about the breaking > > >>>> change. > > >>>> > > >>>> I have added the `release/important-notice ` label to the PR, and we > > >> should > > >>>> also discuss first, better to have a proposal if we are making > > breaking > > >>>> changes. > > >>>> > > >>>> IMO, the main issue here is that the release manager doesn't know the > > >> PR is > > >>>> introducing breaking changes, rather than thinking that the > > >> introduction of > > >>>> breaking changes is reasonable to the patch release. I noticed Jason > > had > > >>>> added the release/* label, I think he also isn't aware of the breaking > > >>>> change. > > >>>> > > >>>> The release manager is unable to review all PRs before releasing it. > > >>>> And the PR title said > > >>>> > > >>>> "[Fix][Tiered Storage] Eagerly Delete Offloaded Segments On Topic > > >>>> Deletion". > > >>>> > > >>>> My impression, it also should be bug fix. > > >>>> > > >>>> Regards, > > >>>> Penghui > > >>>> > > >>>>> On Tue, Feb 28, 2023 at 10:32 AM Xiangying Meng < > > xiangy...@apache.org> > > >>>>> wrote: > > >>>>> > > >>>>> Hi Enrico Olivelli, > > >>>>> > > >>>>> Totally agree, we should be careful when cherry-picking PRs. And we > > >> can't > > >>>>> trust our own judgment too much. For an uncertain PR, we must submit > > a > > >> PR > > >>>>> and wait for everyone to review it together. > > >>>>> For example, for the PR [1] mentioned above, the measure I took was > > to > > >>>> push > > >>>>> a PR to cherry-pick and move it to the next release version (2.10.5) > > so > > >>>>> that we have enough time to discuss and reach an agreement. > > >>>>> > > >>>>> Sincerely, > > >>>>> Xiangying > > >>>>> [1] > > >>>>> > > >> > > https://github.com/apache/pulsar/pull/19640#pullrequestreview-1315805022 > > >>>>> > > >>>>> On Tue, Feb 28, 2023 at 9:56 AM Yubiao Feng > > >>>>> <yubiao.f...@streamnative.io.invalid> wrote: > > >>>>> > > >>>>>> Hi Enrico Olivelli > > >>>>>> > > >>>>>> Thank you for helping me correct my mistake > > >>>>>> > > >>>>>> Yubiao Feng > > >>>>>> > > >>>>>> On Mon, Feb 27, 2023 at 11:27 PM Enrico Olivelli < > > eolive...@gmail.com > > >>> > > >>>>>> wrote: > > >>>>>> > > >>>>>>> Hello Committers, > > >>>>>>> I believe that we should stop cherry-picking breaking changes like > > >>>> [1] > > >>>>>>> to released branches. > > >>>>>>> Really, this is something that we cannot do. > > >>>>>>> > > >>>>>>> When you decide to cherry-pick a commit to a "stable branch", > > >>>>>>> currently branch-2.8, branch-2.9, branch-2.10 and branch-2.11 you > > >>>>>>> always have to think about these things: > > >>>>>>> - is it a breaking change ? > > >>>>>>> - is it really needed ? > > >>>>>>> - could it mine the stability of the branch ? > > >>>>>>> > > >>>>>>> The answer is usually that you can cherry-pick a change only if it > > >>>>>>> falls into these categories: > > >>>>>>> - there is a security hole to fix (in this case the PMC has to deal > > >>>>>>> with it, and usually this is done not publicly) > > >>>>>>> - there is a bad bug that cause data loss or other serious problems > > >>>>>>> > > >>>>>>> I have sent this message a few other times in the past. > > >>>>>>> We, the Pulsar community, are responsible for the stability of the > > >>>>>>> project and product that our users use in production. > > >>>>>>> > > >>>>>>> Even if you think that something that could "improve the > > performance" > > >>>>>>> or "do something better" is appealing you always have to keep in > > mind > > >>>>>>> that the risk of breaking something that is stable is too high in > > >>>>>>> respect to the gain in terms of performances or anything else. > > >>>>>>> > > >>>>>>> Improvements should go only to the master branch, and users will > > >>>>>>> benefit from them when we will cut a release. > > >>>>>>> > > >>>>>>> This is a free OSS project on which many users count on. > > >>>>>>> > > >>>>>>> If you are eager to see a performance improvement in your system, > > >>>> then > > >>>>>>> this is fine, > > >>>>>>> this is OSS and you can legally have a fork and cherry-pick the > > >>>>>>> patches and build it on your own. > > >>>>>>> This is the reason why OSS is cool. > > >>>>>>> But if you are able to cherry-pick a patch you are also able to > > >>>>>>> maintain your fork and fix any problems if the patch caused a > > >>>>>>> regression. > > >>>>>>> > > >>>>>>> Most of the consumers of OSS products rely on us because they don't > > >>>>>>> have enough engineering resources to maintain such a project by > > >>>>>>> themselves. > > >>>>>>> > > >>>>>>> They trust us and they won't scan a list of tens of commits in > > order > > >>>>>>> to double check if the upgrade will change the behaviour of their > > >>>>>>> applications. > > >>>>>>> > > >>>>>>> This is Pulsar momentum, let's do our best to fulfill the > > >>>> expectations > > >>>>>>> of the companies that are adopting our project. > > >>>>>>> > > >>>>>>> Enrico > > >>>>>>> > > >>>>>>> [1] > > >>>>>>> > > >>>>> > > >> > > https://github.com/apache/pulsar/pull/19640#pullrequestreview-1315805022 > > >>>>>>> > > >>>>>> > > >>>>> > > >>>> > > >> > > >> > >
