You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flink.apache.org by Robert Metzger <rm...@apache.org> on 2020/02/17 14:55:46 UTC
Re: Hotfixes on the master
Hi all,
I would like to revive this very old thread on the topic of "unreviewed
hotfixes on master" again.
Out of the 35 commits listed on the latest commits page on GitHub, 18 have
the tag "hotfix", on the next page it is 9, then 16, 17, ...
In the last 140 commits, 42% were hotfixes.
For the sake of this discussion, let's distinguish between two types of
hotfixes:
a) *reviewed hotfix commits*: They have been reviewed through a pull
request, then committed to master.
b) *unreviewed hotfix commits*: These have been pushed straight to master,
without a review.
It's quite difficult to find out whether a hotfix has been reviewed or not
(because many hotfix commits are reviewed & pushed as part of a PR), but
here are some recent examples of commits where I could not find evidence of
a pull request:
// these could probably be combined into on JIRA ticket, as they affect the
same component + they touch dependencies
47a1725ae14a772ba8590ee97dffd7fdf5bc04b2 [hotfix][docs][conf] Log included
packages / excluded classes
a5894677d95336a67d5539584b9204bcdd14fac5 [hotfix][docs][conf] Setup logging
for generator
325927064542c2d018f9da33660c1cdf57e0e382 [hotfix][docs][conf] Add query
service port to port section
3c696a34145e838c046805b36553a50ec9bfbda0 [hotfix][docs][conf] Add query
service port to port section
// dependency change
736ebc0b40abab88902ada3f564777c3ade03001 [hotfix][build] Remove various
unused test dependencies
// more than a regeneration / typo / compile error change
30b5f6173e688ea20b82226db6923db19dec29a5 [hotfix][tests] Adjust
FileUtilsTest.testDeleteSymbolicLinkDirectory() to handle unsupported
situations in Windows
fc59aa4ecc2a7170bfda14ffadf0a30aa2b793bf [FLINK-16065][core] Unignore
FileUtilsTest.testDeleteDirectoryConcurrently()
// dependency changes
fe7145787a7f36b21aad748ffea4ee8ab03c02b7 [hotfix][build] Remove unused
akka-testkit dependencies
dd34b050e8e7bd4b03ad0870a432b1631e1c0e9d [hotfix][build] Remove unused
shaded-asm7 dependencies
// dependency changes
244d2db78307cd7dff1c60a664046adb6fe5c405 [hotfix][web][build] Cleanup
dependencies
In my opinion, everything that is not a typo, a compile error (breaking the
master) or something generated (like parts of the docs) should go through a
quick pull request.
Why? I don't think many people review changes in the commit log in a way
they review pull request changes.
In addition to that, I propose to prefix hotfixes that have been added as
part of a ticket with that ticket number.
So instead of "[hotfix] Harden kubernetes test", we do "[FLINK-13978][minor]
Harden kubernetes test".
Why? For people checking the commit history, it is much easier to see if a
hotfix has been reviewed as part of a JIRA ticket review, or whether it is
a "hotpush" hotfix.
For changes that are too small for a JIRA ticket, but need a review, I
propose to use the "[minor]" tag. A good example of such a change is this:
https://github.com/apache/flink/commit/0dc4e767c9c48ac58430a59d05185f2b071f53f5
My tagging minor changes accordingly in the pull requests, it is also
easier for fellow committers to quickly check them.
Summary:
[FLINK-XXXX]: regular, reviewed change
[FLINK-XXXX][minor]: minor, unrelated changes reviewed with a regular ticket
[minor]: minor, reviewed change
[hotfix]: unreviewed change that fixes a typo, compile error or something
generated
What's your opinion on this?
On Sat, May 28, 2016 at 1:36 PM Vasiliki Kalavri <va...@gmail.com>
wrote:
> Hi all,
>
> in principle I agree with Max. I personally avoid hotfixes and always open
> a PR, even for javadoc improvements.
>
> I believe the main problem is that we don't have a clear definition of what
> constitutes a "hotfix". Ideally, even cosmetic changes and documentation
> should be reviewed; I've seen documentation added as a hotfix that had
> spelling mistakes, which led to another hotfix... Using hotfixes to do
> major refactoring or add features is absolutely unacceptable, in my view.
> On the other hand, with the current PR load it's not practical to ban
> hotfixes all together.
>
> I would suggest to update our contribution guidelines with some definition
> of a hotfix. We could add a list of questions to ask before pushing one.
> e.g.:
> - does the change fix a spelling mistake in the docs? => hotfix
> - does the change add a missing javadoc? => hotfix
> - does the change improve a comment? => hotfix?
> - is the change a small refactoring in a code component you are maintainer
> of? => hotfix
> - did you change code in a component you are not very familiar with / not
> the maintainer of? => open PR
> - is this major refactoring? (e.g. more than X lines of code) => open PR
> - does it fix a trivial bug? => open JIRA and PR
>
> and so on...
>
> What do you think?
>
> Cheers,
> -V.
>
> On 27 May 2016 at 17:40, Greg Hogan <co...@greghogan.com> wrote:
>
> > Max,
> >
> > I certainly agree that hotfixes are not ideal for large refactorings and
> > new features. Some thoughts ...
> >
> > A hotfix should be maven verified, as should a rebased PR. Travis is
> often
> > backed up for half a day or more.
> >
> > Is our Jira and PR process sufficiently agile to handle these hotfixes?
> > Will committers simply include hotfixes with other PRs, and would it be
> > better to retain these as smaller, separate commits?
> >
> > For these cosmetic changes and small updates will the Jira and PR yield
> > beneficial documentation addition to what is provided in the commit
> > message?
> >
> > Counting hotfixes by contributor, the top of the list looks as I would
> > expect.
> >
> > Greg
> >
> > Note: this summary is rather naive and includes non-squashed hotfix
> commits
> > included in a PR
> > $ git shortlog --grep 'hotfix' -s -n release-0.9.0..
> > 94 Stephan Ewen
> > 42 Aljoscha Krettek
> > 20 Till Rohrmann
> > 16 Robert Metzger
> > 13 Ufuk Celebi
> > 9 Fabian Hueske
> > 9 Maximilian Michels
> > 6 Greg Hogan
> > 5 Stefano Baghino
> > 3 smarthi
> > 2 Andrea Sella
> > 2 Gyula Fora
> > 2 Jun Aoki
> > 2 Sachin Goel
> > 2 mjsax
> > 2 zentol
> > 1 Alexander Alexandrov
> > 1 Gabor Gevay
> > 1 Prez Cannady
> > 1 Steve Cosenza
> > 1 Suminda Dharmasena
> > 1 chengxiang li
> > 1 jaoki
> > 1 kl0u
> > 1 qingmeng.wyh
> > 1 sksamuel
> > 1 vasia
> >
> > On Fri, May 27, 2016 at 6:10 AM, Maximilian Michels <mx...@apache.org>
> > wrote:
> >
> > > Hi Flinksters,
> > >
> > > I'd like to address an issue that has been concerning me for a while.
> > > In the Flink community we like to push "hotfixes" to the master.
> > > Hotfixes come in various shapes: From very small cosmetic changes
> > > (JavaDoc) to major refactoring and even new features.
> > >
> > > IMHO we should move away from these hotfixes. Here's why:
> > >
> > > 1) They tend to break the master because they lack test coverage
> > > 2) They are usually not communicated with the maintainer or person
> > > working on the part being fixed
> > > 3) They are not properly documented for future reference or follow-ups
> > > (JIRA/Github)
> > >
> > > That's why I have chosen not to push hotfixes anymore. Even for small
> > > fixes, I'll open a JIRA/Github issue. The only exception might be
> > > fixing a comment. It improves communication, documentation, and test
> > > coverage. All this helps to mature Flink and develop the community in
> > > a transparent way.
> > >
> > > I'm not sure what our contribution guidelines say about this but I
> > > would like to update them to explicitly address hotfixes. Let me know
> > > what you think.
> > >
> > > Best,
> > > Max
> > >
> >
>
Re: Hotfixes on the master
Posted by Chesnay Schepler <ch...@apache.org>.
@Arvid Pretty sure that goes against ASF rules.
On 19/02/2020 14:26, Arvid Heise wrote:
> Let me be a troll, but couldn't we just disable push to master for
> committers?
>
> That would make smaller backports harder, but it would also eliminate
> unreviewed commits. And we wouldn't need to draw a line between minor for
> direct push and bigger changes that need review.
>
> On Wed, Feb 19, 2020 at 1:27 PM Piotr Nowojski <pi...@ververica.com> wrote:
>
>> Hi,
>>
>> I agree with question posted by Till, what problems are we trying to solve?
>>
>> Regarding merging unreviewed commits, I’m +1 for disallowing those things.
>> Anything that is trivial enough that I would accept as being merged without
>> review, is also trivial enough to review. The additional review overhead is
>> for me just too small compared to an extra pair of eyes that can catch
>> something that seemed trivial for me, but it wasn’t.
>>
>> Regarding the [minor] infix… I don’t like it as it makes commit messages
>> even longer (`[FLINK-12345][minor][runtime][tests]` oO?) and I didn’t
>> notice any big issues with reviewed `[hotifx]` commits. For one thing, it
>> is super rare for me to wonder which PR did this hotfix belong. And even if
>> I do this, just looking for some jira ticket around the notify with the
>> same author should be enough. But all in all, it’s not that longer commit
>> messages that big of a deal (just GitHub’s UI doesn’t like it).
>>
>> Piotrek
>>
>>> Side comment: the problem of not knowing where a commit came from could
>> be fixed by always including merge commits, just saying... ;-)
>>
>> Blasphemy
>>
>>> On 19 Feb 2020, at 12:55, Aljoscha Krettek <al...@apache.org> wrote:
>>>
>>> Side comment: the problem of not knowing where a commit came from could
>> be fixed by always including merge commits, just saying... ;-)
>>> Aljoscha
>>>
>>> On 19.02.20 11:43, Robert Metzger wrote:
>>>> Thank you for your response. Your response makes me realize that I
>> should
>>>> have first asked whether other committers consider the amount of hotfix
>>>> commits problematic or not.
>>>> From the number of responses to my message, I have the feeling that most
>>>> committers are not concerned.
>>>> I personally believe that the amount of hotfix commits is too high.
>> There
>>>> have been a few cases of hotfixes committed recently that have been
>>>> commented by other committers, have been reverted quickly after or
>> amended
>>>> in another hotfix. My hope is that a proper PR review would have caught
>>>> some of those cases.
>>>> Additionally, there's almost no papertrail around hotfixes.
>>>> Regular commits have a JIRA ticket, which usually has some problem
>>>> statement and ideally some discussion, and a pull request review.
>>>> Since we are using the Merge button in PRs quite frequently, we also
>> don't
>>>> have the "This closes #12345" message in the commit anymore, which
>> would at
>>>> least point to the review PR ... so when I'm checking the commit
>> history, I
>>>> need to manually search the PR history as well, to see if there is more
>>>> context on a commit in a PR review or not. (Hence my minor vs hotfix
>>>> proposal)
>>>> I'm happy to leave things as-is for now. As long as there are still
>> people
>>>> catching mistakes made in hotfix commits, I don't see a decline in
>> quality.
>>>> On Tue, Feb 18, 2020 at 7:58 PM Till Rohrmann <tr...@apache.org>
>> wrote:
>>>>> Thanks for raising this point Robert. I think it is important to
>> remind the
>>>>> community about the agreed practices once in a while.
>>>>>
>>>>> In most of the cases I had the impression that the majority of the
>>>>> community sticks to the agreed rules. W/o more detailed numbers (how
>> many
>>>>> of the hotfix commits are of category b) I think this discussion makes
>> only
>>>>> limited sense. Moreover, what is the underlying problem you would like
>> to
>>>>> solve here? Is it that too many commits have the hotfix tag in the
>> commit
>>>>> log? Is it that it's hard to figure out with which PR a hotfix commit
>> has
>>>>> been merged? Or did you observe a decline in Flink's quality because
>> of too
>>>>> many unreviewed changes? If we are discussing the latter case, then I
>> think
>>>>> it is very urgent. In the former cases I'm not entirely sure whether
>> this
>>>>> is an immediate problem because from what I have seen people include
>> many
>>>>> more clean up/hotfix commits in their PRs these days.
>>>>>
>>>>> Concerning the proposed practice with the [minor] tag I'm a bit torn.
>> The
>>>>> benefit of not doing it like this is that it's easier to see which
>> commits
>>>>> need to be cherry-picked if a feature needs to be ported, for example.
>> On
>>>>> the other hand if the commits are not unrelated and the feature
>> commits use
>>>>> parts of the hotfix commits, then it makes the cherry-picking more
>> tricky
>>>>> and the commits should have the JIRA issue tag. But I would be fine
>> with
>>>>> trying it out.
>>>>>
>>>>> Cheers,
>>>>> Till
>>>>>
>>>>> On Mon, Feb 17, 2020 at 3:56 PM Robert Metzger <rm...@apache.org>
>>>>> wrote:
>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> I would like to revive this very old thread on the topic of
>> "unreviewed
>>>>>> hotfixes on master" again.
>>>>>> Out of the 35 commits listed on the latest commits page on GitHub, 18
>>>>> have
>>>>>> the tag "hotfix", on the next page it is 9, then 16, 17, ...
>>>>>> In the last 140 commits, 42% were hotfixes.
>>>>>>
>>>>>> For the sake of this discussion, let's distinguish between two types
>> of
>>>>>> hotfixes:
>>>>>> a) *reviewed hotfix commits*: They have been reviewed through a pull
>>>>>> request, then committed to master.
>>>>>> b) *unreviewed hotfix commits*: These have been pushed straight to
>>>>> master,
>>>>>> without a review.
>>>>>>
>>>>>> It's quite difficult to find out whether a hotfix has been reviewed or
>>>>> not
>>>>>> (because many hotfix commits are reviewed & pushed as part of a PR),
>> but
>>>>>> here are some recent examples of commits where I could not find
>> evidence
>>>>> of
>>>>>> a pull request:
>>>>>>
>>>>>> // these could probably be combined into on JIRA ticket, as they
>> affect
>>>>> the
>>>>>> same component + they touch dependencies
>>>>>> 47a1725ae14a772ba8590ee97dffd7fdf5bc04b2 [hotfix][docs][conf] Log
>>>>> included
>>>>>> packages / excluded classes
>>>>>> a5894677d95336a67d5539584b9204bcdd14fac5 [hotfix][docs][conf] Setup
>>>>> logging
>>>>>> for generator
>>>>>> 325927064542c2d018f9da33660c1cdf57e0e382 [hotfix][docs][conf] Add
>> query
>>>>>> service port to port section
>>>>>> 3c696a34145e838c046805b36553a50ec9bfbda0 [hotfix][docs][conf] Add
>> query
>>>>>> service port to port section
>>>>>>
>>>>>> // dependency change
>>>>>> 736ebc0b40abab88902ada3f564777c3ade03001 [hotfix][build] Remove
>> various
>>>>>> unused test dependencies
>>>>>>
>>>>>> // more than a regeneration / typo / compile error change
>>>>>> 30b5f6173e688ea20b82226db6923db19dec29a5 [hotfix][tests] Adjust
>>>>>> FileUtilsTest.testDeleteSymbolicLinkDirectory() to handle unsupported
>>>>>> situations in Windows
>>>>>> fc59aa4ecc2a7170bfda14ffadf0a30aa2b793bf [FLINK-16065][core] Unignore
>>>>>> FileUtilsTest.testDeleteDirectoryConcurrently()
>>>>>>
>>>>>> // dependency changes
>>>>>> fe7145787a7f36b21aad748ffea4ee8ab03c02b7 [hotfix][build] Remove unused
>>>>>> akka-testkit dependencies
>>>>>> dd34b050e8e7bd4b03ad0870a432b1631e1c0e9d [hotfix][build] Remove unused
>>>>>> shaded-asm7 dependencies
>>>>>>
>>>>>> // dependency changes
>>>>>> 244d2db78307cd7dff1c60a664046adb6fe5c405 [hotfix][web][build] Cleanup
>>>>>> dependencies
>>>>>>
>>>>>> In my opinion, everything that is not a typo, a compile error
>> (breaking
>>>>> the
>>>>>> master) or something generated (like parts of the docs) should go
>>>>> through a
>>>>>> quick pull request.
>>>>>> Why? I don't think many people review changes in the commit log in a
>> way
>>>>>> they review pull request changes.
>>>>>>
>>>>>> In addition to that, I propose to prefix hotfixes that have been
>> added as
>>>>>> part of a ticket with that ticket number.
>>>>>> So instead of "[hotfix] Harden kubernetes test", we do
>>>>>> "[FLINK-13978][minor]
>>>>>> Harden kubernetes test".
>>>>>> Why? For people checking the commit history, it is much easier to see
>> if
>>>>> a
>>>>>> hotfix has been reviewed as part of a JIRA ticket review, or whether
>> it
>>>>> is
>>>>>> a "hotpush" hotfix.
>>>>>>
>>>>>> For changes that are too small for a JIRA ticket, but need a review, I
>>>>>> propose to use the "[minor]" tag. A good example of such a change is
>>>>> this:
>>>>>>
>> https://github.com/apache/flink/commit/0dc4e767c9c48ac58430a59d05185f2b071f53f5
>>>>>> My tagging minor changes accordingly in the pull requests, it is also
>>>>>> easier for fellow committers to quickly check them.
>>>>>>
>>>>>> Summary:
>>>>>> [FLINK-XXXX]: regular, reviewed change
>>>>>> [FLINK-XXXX][minor]: minor, unrelated changes reviewed with a regular
>>>>>> ticket
>>>>>> [minor]: minor, reviewed change
>>>>>> [hotfix]: unreviewed change that fixes a typo, compile error or
>> something
>>>>>> generated
>>>>>>
>>>>>>
>>>>>> What's your opinion on this?
>>>>>>
>>>>>>
>>>>>> On Sat, May 28, 2016 at 1:36 PM Vasiliki Kalavri <
>>>>>> vasilikikalavri@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> in principle I agree with Max. I personally avoid hotfixes and always
>>>>>> open
>>>>>>> a PR, even for javadoc improvements.
>>>>>>>
>>>>>>> I believe the main problem is that we don't have a clear definition
>> of
>>>>>> what
>>>>>>> constitutes a "hotfix". Ideally, even cosmetic changes and
>>>>> documentation
>>>>>>> should be reviewed; I've seen documentation added as a hotfix that
>> had
>>>>>>> spelling mistakes, which led to another hotfix... Using hotfixes to
>> do
>>>>>>> major refactoring or add features is absolutely unacceptable, in my
>>>>> view.
>>>>>>> On the other hand, with the current PR load it's not practical to ban
>>>>>>> hotfixes all together.
>>>>>>>
>>>>>>> I would suggest to update our contribution guidelines with some
>>>>>> definition
>>>>>>> of a hotfix. We could add a list of questions to ask before pushing
>>>>> one.
>>>>>>> e.g.:
>>>>>>> - does the change fix a spelling mistake in the docs? => hotfix
>>>>>>> - does the change add a missing javadoc? => hotfix
>>>>>>> - does the change improve a comment? => hotfix?
>>>>>>> - is the change a small refactoring in a code component you are
>>>>>> maintainer
>>>>>>> of? => hotfix
>>>>>>> - did you change code in a component you are not very familiar with /
>>>>> not
>>>>>>> the maintainer of? => open PR
>>>>>>> - is this major refactoring? (e.g. more than X lines of code) => open
>>>>> PR
>>>>>>> - does it fix a trivial bug? => open JIRA and PR
>>>>>>>
>>>>>>> and so on...
>>>>>>>
>>>>>>> What do you think?
>>>>>>>
>>>>>>> Cheers,
>>>>>>> -V.
>>>>>>>
>>>>>>> On 27 May 2016 at 17:40, Greg Hogan <co...@greghogan.com> wrote:
>>>>>>>
>>>>>>>> Max,
>>>>>>>>
>>>>>>>> I certainly agree that hotfixes are not ideal for large refactorings
>>>>>> and
>>>>>>>> new features. Some thoughts ...
>>>>>>>>
>>>>>>>> A hotfix should be maven verified, as should a rebased PR. Travis is
>>>>>>> often
>>>>>>>> backed up for half a day or more.
>>>>>>>>
>>>>>>>> Is our Jira and PR process sufficiently agile to handle these
>>>>> hotfixes?
>>>>>>>> Will committers simply include hotfixes with other PRs, and would it
>>>>> be
>>>>>>>> better to retain these as smaller, separate commits?
>>>>>>>>
>>>>>>>> For these cosmetic changes and small updates will the Jira and PR
>>>>> yield
>>>>>>>> beneficial documentation addition to what is provided in the commit
>>>>>>>> message?
>>>>>>>>
>>>>>>>> Counting hotfixes by contributor, the top of the list looks as I
>>>>> would
>>>>>>>> expect.
>>>>>>>>
>>>>>>>> Greg
>>>>>>>>
>>>>>>>> Note: this summary is rather naive and includes non-squashed hotfix
>>>>>>> commits
>>>>>>>> included in a PR
>>>>>>>> $ git shortlog --grep 'hotfix' -s -n release-0.9.0..
>>>>>>>> 94 Stephan Ewen
>>>>>>>> 42 Aljoscha Krettek
>>>>>>>> 20 Till Rohrmann
>>>>>>>> 16 Robert Metzger
>>>>>>>> 13 Ufuk Celebi
>>>>>>>> 9 Fabian Hueske
>>>>>>>> 9 Maximilian Michels
>>>>>>>> 6 Greg Hogan
>>>>>>>> 5 Stefano Baghino
>>>>>>>> 3 smarthi
>>>>>>>> 2 Andrea Sella
>>>>>>>> 2 Gyula Fora
>>>>>>>> 2 Jun Aoki
>>>>>>>> 2 Sachin Goel
>>>>>>>> 2 mjsax
>>>>>>>> 2 zentol
>>>>>>>> 1 Alexander Alexandrov
>>>>>>>> 1 Gabor Gevay
>>>>>>>> 1 Prez Cannady
>>>>>>>> 1 Steve Cosenza
>>>>>>>> 1 Suminda Dharmasena
>>>>>>>> 1 chengxiang li
>>>>>>>> 1 jaoki
>>>>>>>> 1 kl0u
>>>>>>>> 1 qingmeng.wyh
>>>>>>>> 1 sksamuel
>>>>>>>> 1 vasia
>>>>>>>>
>>>>>>>> On Fri, May 27, 2016 at 6:10 AM, Maximilian Michels <mxm@apache.org
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Hi Flinksters,
>>>>>>>>>
>>>>>>>>> I'd like to address an issue that has been concerning me for a
>>>>> while.
>>>>>>>>> In the Flink community we like to push "hotfixes" to the master.
>>>>>>>>> Hotfixes come in various shapes: From very small cosmetic changes
>>>>>>>>> (JavaDoc) to major refactoring and even new features.
>>>>>>>>>
>>>>>>>>> IMHO we should move away from these hotfixes. Here's why:
>>>>>>>>>
>>>>>>>>> 1) They tend to break the master because they lack test coverage
>>>>>>>>> 2) They are usually not communicated with the maintainer or person
>>>>>>>>> working on the part being fixed
>>>>>>>>> 3) They are not properly documented for future reference or
>>>>>> follow-ups
>>>>>>>>> (JIRA/Github)
>>>>>>>>>
>>>>>>>>> That's why I have chosen not to push hotfixes anymore. Even for
>>>>> small
>>>>>>>>> fixes, I'll open a JIRA/Github issue. The only exception might be
>>>>>>>>> fixing a comment. It improves communication, documentation, and
>>>>> test
>>>>>>>>> coverage. All this helps to mature Flink and develop the community
>>>>> in
>>>>>>>>> a transparent way.
>>>>>>>>>
>>>>>>>>> I'm not sure what our contribution guidelines say about this but I
>>>>>>>>> would like to update them to explicitly address hotfixes. Let me
>>>>> know
>>>>>>>>> what you think.
>>>>>>>>>
>>>>>>>>> Best,
>>>>>>>>> Max
>>>>>>>>>
>>
Re: Hotfixes on the master
Posted by Arvid Heise <ar...@ververica.com>.
Let me be a troll, but couldn't we just disable push to master for
committers?
That would make smaller backports harder, but it would also eliminate
unreviewed commits. And we wouldn't need to draw a line between minor for
direct push and bigger changes that need review.
On Wed, Feb 19, 2020 at 1:27 PM Piotr Nowojski <pi...@ververica.com> wrote:
> Hi,
>
> I agree with question posted by Till, what problems are we trying to solve?
>
> Regarding merging unreviewed commits, I’m +1 for disallowing those things.
> Anything that is trivial enough that I would accept as being merged without
> review, is also trivial enough to review. The additional review overhead is
> for me just too small compared to an extra pair of eyes that can catch
> something that seemed trivial for me, but it wasn’t.
>
> Regarding the [minor] infix… I don’t like it as it makes commit messages
> even longer (`[FLINK-12345][minor][runtime][tests]` oO?) and I didn’t
> notice any big issues with reviewed `[hotifx]` commits. For one thing, it
> is super rare for me to wonder which PR did this hotfix belong. And even if
> I do this, just looking for some jira ticket around the notify with the
> same author should be enough. But all in all, it’s not that longer commit
> messages that big of a deal (just GitHub’s UI doesn’t like it).
>
> Piotrek
>
> > Side comment: the problem of not knowing where a commit came from could
> be fixed by always including merge commits, just saying... ;-)
>
> Blasphemy
>
> > On 19 Feb 2020, at 12:55, Aljoscha Krettek <al...@apache.org> wrote:
> >
> > Side comment: the problem of not knowing where a commit came from could
> be fixed by always including merge commits, just saying... ;-)
> >
> > Aljoscha
> >
> > On 19.02.20 11:43, Robert Metzger wrote:
> >> Thank you for your response. Your response makes me realize that I
> should
> >> have first asked whether other committers consider the amount of hotfix
> >> commits problematic or not.
> >> From the number of responses to my message, I have the feeling that most
> >> committers are not concerned.
> >> I personally believe that the amount of hotfix commits is too high.
> There
> >> have been a few cases of hotfixes committed recently that have been
> >> commented by other committers, have been reverted quickly after or
> amended
> >> in another hotfix. My hope is that a proper PR review would have caught
> >> some of those cases.
> >> Additionally, there's almost no papertrail around hotfixes.
> >> Regular commits have a JIRA ticket, which usually has some problem
> >> statement and ideally some discussion, and a pull request review.
> >> Since we are using the Merge button in PRs quite frequently, we also
> don't
> >> have the "This closes #12345" message in the commit anymore, which
> would at
> >> least point to the review PR ... so when I'm checking the commit
> history, I
> >> need to manually search the PR history as well, to see if there is more
> >> context on a commit in a PR review or not. (Hence my minor vs hotfix
> >> proposal)
> >> I'm happy to leave things as-is for now. As long as there are still
> people
> >> catching mistakes made in hotfix commits, I don't see a decline in
> quality.
> >> On Tue, Feb 18, 2020 at 7:58 PM Till Rohrmann <tr...@apache.org>
> wrote:
> >>> Thanks for raising this point Robert. I think it is important to
> remind the
> >>> community about the agreed practices once in a while.
> >>>
> >>> In most of the cases I had the impression that the majority of the
> >>> community sticks to the agreed rules. W/o more detailed numbers (how
> many
> >>> of the hotfix commits are of category b) I think this discussion makes
> only
> >>> limited sense. Moreover, what is the underlying problem you would like
> to
> >>> solve here? Is it that too many commits have the hotfix tag in the
> commit
> >>> log? Is it that it's hard to figure out with which PR a hotfix commit
> has
> >>> been merged? Or did you observe a decline in Flink's quality because
> of too
> >>> many unreviewed changes? If we are discussing the latter case, then I
> think
> >>> it is very urgent. In the former cases I'm not entirely sure whether
> this
> >>> is an immediate problem because from what I have seen people include
> many
> >>> more clean up/hotfix commits in their PRs these days.
> >>>
> >>> Concerning the proposed practice with the [minor] tag I'm a bit torn.
> The
> >>> benefit of not doing it like this is that it's easier to see which
> commits
> >>> need to be cherry-picked if a feature needs to be ported, for example.
> On
> >>> the other hand if the commits are not unrelated and the feature
> commits use
> >>> parts of the hotfix commits, then it makes the cherry-picking more
> tricky
> >>> and the commits should have the JIRA issue tag. But I would be fine
> with
> >>> trying it out.
> >>>
> >>> Cheers,
> >>> Till
> >>>
> >>> On Mon, Feb 17, 2020 at 3:56 PM Robert Metzger <rm...@apache.org>
> >>> wrote:
> >>>
> >>>> Hi all,
> >>>>
> >>>> I would like to revive this very old thread on the topic of
> "unreviewed
> >>>> hotfixes on master" again.
> >>>> Out of the 35 commits listed on the latest commits page on GitHub, 18
> >>> have
> >>>> the tag "hotfix", on the next page it is 9, then 16, 17, ...
> >>>> In the last 140 commits, 42% were hotfixes.
> >>>>
> >>>> For the sake of this discussion, let's distinguish between two types
> of
> >>>> hotfixes:
> >>>> a) *reviewed hotfix commits*: They have been reviewed through a pull
> >>>> request, then committed to master.
> >>>> b) *unreviewed hotfix commits*: These have been pushed straight to
> >>> master,
> >>>> without a review.
> >>>>
> >>>> It's quite difficult to find out whether a hotfix has been reviewed or
> >>> not
> >>>> (because many hotfix commits are reviewed & pushed as part of a PR),
> but
> >>>> here are some recent examples of commits where I could not find
> evidence
> >>> of
> >>>> a pull request:
> >>>>
> >>>> // these could probably be combined into on JIRA ticket, as they
> affect
> >>> the
> >>>> same component + they touch dependencies
> >>>> 47a1725ae14a772ba8590ee97dffd7fdf5bc04b2 [hotfix][docs][conf] Log
> >>> included
> >>>> packages / excluded classes
> >>>> a5894677d95336a67d5539584b9204bcdd14fac5 [hotfix][docs][conf] Setup
> >>> logging
> >>>> for generator
> >>>> 325927064542c2d018f9da33660c1cdf57e0e382 [hotfix][docs][conf] Add
> query
> >>>> service port to port section
> >>>> 3c696a34145e838c046805b36553a50ec9bfbda0 [hotfix][docs][conf] Add
> query
> >>>> service port to port section
> >>>>
> >>>> // dependency change
> >>>> 736ebc0b40abab88902ada3f564777c3ade03001 [hotfix][build] Remove
> various
> >>>> unused test dependencies
> >>>>
> >>>> // more than a regeneration / typo / compile error change
> >>>> 30b5f6173e688ea20b82226db6923db19dec29a5 [hotfix][tests] Adjust
> >>>> FileUtilsTest.testDeleteSymbolicLinkDirectory() to handle unsupported
> >>>> situations in Windows
> >>>> fc59aa4ecc2a7170bfda14ffadf0a30aa2b793bf [FLINK-16065][core] Unignore
> >>>> FileUtilsTest.testDeleteDirectoryConcurrently()
> >>>>
> >>>> // dependency changes
> >>>> fe7145787a7f36b21aad748ffea4ee8ab03c02b7 [hotfix][build] Remove unused
> >>>> akka-testkit dependencies
> >>>> dd34b050e8e7bd4b03ad0870a432b1631e1c0e9d [hotfix][build] Remove unused
> >>>> shaded-asm7 dependencies
> >>>>
> >>>> // dependency changes
> >>>> 244d2db78307cd7dff1c60a664046adb6fe5c405 [hotfix][web][build] Cleanup
> >>>> dependencies
> >>>>
> >>>> In my opinion, everything that is not a typo, a compile error
> (breaking
> >>> the
> >>>> master) or something generated (like parts of the docs) should go
> >>> through a
> >>>> quick pull request.
> >>>> Why? I don't think many people review changes in the commit log in a
> way
> >>>> they review pull request changes.
> >>>>
> >>>> In addition to that, I propose to prefix hotfixes that have been
> added as
> >>>> part of a ticket with that ticket number.
> >>>> So instead of "[hotfix] Harden kubernetes test", we do
> >>>> "[FLINK-13978][minor]
> >>>> Harden kubernetes test".
> >>>> Why? For people checking the commit history, it is much easier to see
> if
> >>> a
> >>>> hotfix has been reviewed as part of a JIRA ticket review, or whether
> it
> >>> is
> >>>> a "hotpush" hotfix.
> >>>>
> >>>> For changes that are too small for a JIRA ticket, but need a review, I
> >>>> propose to use the "[minor]" tag. A good example of such a change is
> >>> this:
> >>>>
> >>>>
> >>>
> https://github.com/apache/flink/commit/0dc4e767c9c48ac58430a59d05185f2b071f53f5
> >>>>
> >>>> My tagging minor changes accordingly in the pull requests, it is also
> >>>> easier for fellow committers to quickly check them.
> >>>>
> >>>> Summary:
> >>>> [FLINK-XXXX]: regular, reviewed change
> >>>> [FLINK-XXXX][minor]: minor, unrelated changes reviewed with a regular
> >>>> ticket
> >>>> [minor]: minor, reviewed change
> >>>> [hotfix]: unreviewed change that fixes a typo, compile error or
> something
> >>>> generated
> >>>>
> >>>>
> >>>> What's your opinion on this?
> >>>>
> >>>>
> >>>> On Sat, May 28, 2016 at 1:36 PM Vasiliki Kalavri <
> >>>> vasilikikalavri@gmail.com>
> >>>> wrote:
> >>>>
> >>>>> Hi all,
> >>>>>
> >>>>> in principle I agree with Max. I personally avoid hotfixes and always
> >>>> open
> >>>>> a PR, even for javadoc improvements.
> >>>>>
> >>>>> I believe the main problem is that we don't have a clear definition
> of
> >>>> what
> >>>>> constitutes a "hotfix". Ideally, even cosmetic changes and
> >>> documentation
> >>>>> should be reviewed; I've seen documentation added as a hotfix that
> had
> >>>>> spelling mistakes, which led to another hotfix... Using hotfixes to
> do
> >>>>> major refactoring or add features is absolutely unacceptable, in my
> >>> view.
> >>>>> On the other hand, with the current PR load it's not practical to ban
> >>>>> hotfixes all together.
> >>>>>
> >>>>> I would suggest to update our contribution guidelines with some
> >>>> definition
> >>>>> of a hotfix. We could add a list of questions to ask before pushing
> >>> one.
> >>>>> e.g.:
> >>>>> - does the change fix a spelling mistake in the docs? => hotfix
> >>>>> - does the change add a missing javadoc? => hotfix
> >>>>> - does the change improve a comment? => hotfix?
> >>>>> - is the change a small refactoring in a code component you are
> >>>> maintainer
> >>>>> of? => hotfix
> >>>>> - did you change code in a component you are not very familiar with /
> >>> not
> >>>>> the maintainer of? => open PR
> >>>>> - is this major refactoring? (e.g. more than X lines of code) => open
> >>> PR
> >>>>> - does it fix a trivial bug? => open JIRA and PR
> >>>>>
> >>>>> and so on...
> >>>>>
> >>>>> What do you think?
> >>>>>
> >>>>> Cheers,
> >>>>> -V.
> >>>>>
> >>>>> On 27 May 2016 at 17:40, Greg Hogan <co...@greghogan.com> wrote:
> >>>>>
> >>>>>> Max,
> >>>>>>
> >>>>>> I certainly agree that hotfixes are not ideal for large refactorings
> >>>> and
> >>>>>> new features. Some thoughts ...
> >>>>>>
> >>>>>> A hotfix should be maven verified, as should a rebased PR. Travis is
> >>>>> often
> >>>>>> backed up for half a day or more.
> >>>>>>
> >>>>>> Is our Jira and PR process sufficiently agile to handle these
> >>> hotfixes?
> >>>>>> Will committers simply include hotfixes with other PRs, and would it
> >>> be
> >>>>>> better to retain these as smaller, separate commits?
> >>>>>>
> >>>>>> For these cosmetic changes and small updates will the Jira and PR
> >>> yield
> >>>>>> beneficial documentation addition to what is provided in the commit
> >>>>>> message?
> >>>>>>
> >>>>>> Counting hotfixes by contributor, the top of the list looks as I
> >>> would
> >>>>>> expect.
> >>>>>>
> >>>>>> Greg
> >>>>>>
> >>>>>> Note: this summary is rather naive and includes non-squashed hotfix
> >>>>> commits
> >>>>>> included in a PR
> >>>>>> $ git shortlog --grep 'hotfix' -s -n release-0.9.0..
> >>>>>> 94 Stephan Ewen
> >>>>>> 42 Aljoscha Krettek
> >>>>>> 20 Till Rohrmann
> >>>>>> 16 Robert Metzger
> >>>>>> 13 Ufuk Celebi
> >>>>>> 9 Fabian Hueske
> >>>>>> 9 Maximilian Michels
> >>>>>> 6 Greg Hogan
> >>>>>> 5 Stefano Baghino
> >>>>>> 3 smarthi
> >>>>>> 2 Andrea Sella
> >>>>>> 2 Gyula Fora
> >>>>>> 2 Jun Aoki
> >>>>>> 2 Sachin Goel
> >>>>>> 2 mjsax
> >>>>>> 2 zentol
> >>>>>> 1 Alexander Alexandrov
> >>>>>> 1 Gabor Gevay
> >>>>>> 1 Prez Cannady
> >>>>>> 1 Steve Cosenza
> >>>>>> 1 Suminda Dharmasena
> >>>>>> 1 chengxiang li
> >>>>>> 1 jaoki
> >>>>>> 1 kl0u
> >>>>>> 1 qingmeng.wyh
> >>>>>> 1 sksamuel
> >>>>>> 1 vasia
> >>>>>>
> >>>>>> On Fri, May 27, 2016 at 6:10 AM, Maximilian Michels <mxm@apache.org
> >
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Hi Flinksters,
> >>>>>>>
> >>>>>>> I'd like to address an issue that has been concerning me for a
> >>> while.
> >>>>>>> In the Flink community we like to push "hotfixes" to the master.
> >>>>>>> Hotfixes come in various shapes: From very small cosmetic changes
> >>>>>>> (JavaDoc) to major refactoring and even new features.
> >>>>>>>
> >>>>>>> IMHO we should move away from these hotfixes. Here's why:
> >>>>>>>
> >>>>>>> 1) They tend to break the master because they lack test coverage
> >>>>>>> 2) They are usually not communicated with the maintainer or person
> >>>>>>> working on the part being fixed
> >>>>>>> 3) They are not properly documented for future reference or
> >>>> follow-ups
> >>>>>>> (JIRA/Github)
> >>>>>>>
> >>>>>>> That's why I have chosen not to push hotfixes anymore. Even for
> >>> small
> >>>>>>> fixes, I'll open a JIRA/Github issue. The only exception might be
> >>>>>>> fixing a comment. It improves communication, documentation, and
> >>> test
> >>>>>>> coverage. All this helps to mature Flink and develop the community
> >>> in
> >>>>>>> a transparent way.
> >>>>>>>
> >>>>>>> I'm not sure what our contribution guidelines say about this but I
> >>>>>>> would like to update them to explicitly address hotfixes. Let me
> >>> know
> >>>>>>> what you think.
> >>>>>>>
> >>>>>>> Best,
> >>>>>>> Max
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
>
>
Re: Hotfixes on the master
Posted by Piotr Nowojski <pi...@ververica.com>.
Hi,
I agree with question posted by Till, what problems are we trying to solve?
Regarding merging unreviewed commits, I’m +1 for disallowing those things. Anything that is trivial enough that I would accept as being merged without review, is also trivial enough to review. The additional review overhead is for me just too small compared to an extra pair of eyes that can catch something that seemed trivial for me, but it wasn’t.
Regarding the [minor] infix… I don’t like it as it makes commit messages even longer (`[FLINK-12345][minor][runtime][tests]` oO?) and I didn’t notice any big issues with reviewed `[hotifx]` commits. For one thing, it is super rare for me to wonder which PR did this hotfix belong. And even if I do this, just looking for some jira ticket around the notify with the same author should be enough. But all in all, it’s not that longer commit messages that big of a deal (just GitHub’s UI doesn’t like it).
Piotrek
> Side comment: the problem of not knowing where a commit came from could be fixed by always including merge commits, just saying... ;-)
Blasphemy
> On 19 Feb 2020, at 12:55, Aljoscha Krettek <al...@apache.org> wrote:
>
> Side comment: the problem of not knowing where a commit came from could be fixed by always including merge commits, just saying... ;-)
>
> Aljoscha
>
> On 19.02.20 11:43, Robert Metzger wrote:
>> Thank you for your response. Your response makes me realize that I should
>> have first asked whether other committers consider the amount of hotfix
>> commits problematic or not.
>> From the number of responses to my message, I have the feeling that most
>> committers are not concerned.
>> I personally believe that the amount of hotfix commits is too high. There
>> have been a few cases of hotfixes committed recently that have been
>> commented by other committers, have been reverted quickly after or amended
>> in another hotfix. My hope is that a proper PR review would have caught
>> some of those cases.
>> Additionally, there's almost no papertrail around hotfixes.
>> Regular commits have a JIRA ticket, which usually has some problem
>> statement and ideally some discussion, and a pull request review.
>> Since we are using the Merge button in PRs quite frequently, we also don't
>> have the "This closes #12345" message in the commit anymore, which would at
>> least point to the review PR ... so when I'm checking the commit history, I
>> need to manually search the PR history as well, to see if there is more
>> context on a commit in a PR review or not. (Hence my minor vs hotfix
>> proposal)
>> I'm happy to leave things as-is for now. As long as there are still people
>> catching mistakes made in hotfix commits, I don't see a decline in quality.
>> On Tue, Feb 18, 2020 at 7:58 PM Till Rohrmann <tr...@apache.org> wrote:
>>> Thanks for raising this point Robert. I think it is important to remind the
>>> community about the agreed practices once in a while.
>>>
>>> In most of the cases I had the impression that the majority of the
>>> community sticks to the agreed rules. W/o more detailed numbers (how many
>>> of the hotfix commits are of category b) I think this discussion makes only
>>> limited sense. Moreover, what is the underlying problem you would like to
>>> solve here? Is it that too many commits have the hotfix tag in the commit
>>> log? Is it that it's hard to figure out with which PR a hotfix commit has
>>> been merged? Or did you observe a decline in Flink's quality because of too
>>> many unreviewed changes? If we are discussing the latter case, then I think
>>> it is very urgent. In the former cases I'm not entirely sure whether this
>>> is an immediate problem because from what I have seen people include many
>>> more clean up/hotfix commits in their PRs these days.
>>>
>>> Concerning the proposed practice with the [minor] tag I'm a bit torn. The
>>> benefit of not doing it like this is that it's easier to see which commits
>>> need to be cherry-picked if a feature needs to be ported, for example. On
>>> the other hand if the commits are not unrelated and the feature commits use
>>> parts of the hotfix commits, then it makes the cherry-picking more tricky
>>> and the commits should have the JIRA issue tag. But I would be fine with
>>> trying it out.
>>>
>>> Cheers,
>>> Till
>>>
>>> On Mon, Feb 17, 2020 at 3:56 PM Robert Metzger <rm...@apache.org>
>>> wrote:
>>>
>>>> Hi all,
>>>>
>>>> I would like to revive this very old thread on the topic of "unreviewed
>>>> hotfixes on master" again.
>>>> Out of the 35 commits listed on the latest commits page on GitHub, 18
>>> have
>>>> the tag "hotfix", on the next page it is 9, then 16, 17, ...
>>>> In the last 140 commits, 42% were hotfixes.
>>>>
>>>> For the sake of this discussion, let's distinguish between two types of
>>>> hotfixes:
>>>> a) *reviewed hotfix commits*: They have been reviewed through a pull
>>>> request, then committed to master.
>>>> b) *unreviewed hotfix commits*: These have been pushed straight to
>>> master,
>>>> without a review.
>>>>
>>>> It's quite difficult to find out whether a hotfix has been reviewed or
>>> not
>>>> (because many hotfix commits are reviewed & pushed as part of a PR), but
>>>> here are some recent examples of commits where I could not find evidence
>>> of
>>>> a pull request:
>>>>
>>>> // these could probably be combined into on JIRA ticket, as they affect
>>> the
>>>> same component + they touch dependencies
>>>> 47a1725ae14a772ba8590ee97dffd7fdf5bc04b2 [hotfix][docs][conf] Log
>>> included
>>>> packages / excluded classes
>>>> a5894677d95336a67d5539584b9204bcdd14fac5 [hotfix][docs][conf] Setup
>>> logging
>>>> for generator
>>>> 325927064542c2d018f9da33660c1cdf57e0e382 [hotfix][docs][conf] Add query
>>>> service port to port section
>>>> 3c696a34145e838c046805b36553a50ec9bfbda0 [hotfix][docs][conf] Add query
>>>> service port to port section
>>>>
>>>> // dependency change
>>>> 736ebc0b40abab88902ada3f564777c3ade03001 [hotfix][build] Remove various
>>>> unused test dependencies
>>>>
>>>> // more than a regeneration / typo / compile error change
>>>> 30b5f6173e688ea20b82226db6923db19dec29a5 [hotfix][tests] Adjust
>>>> FileUtilsTest.testDeleteSymbolicLinkDirectory() to handle unsupported
>>>> situations in Windows
>>>> fc59aa4ecc2a7170bfda14ffadf0a30aa2b793bf [FLINK-16065][core] Unignore
>>>> FileUtilsTest.testDeleteDirectoryConcurrently()
>>>>
>>>> // dependency changes
>>>> fe7145787a7f36b21aad748ffea4ee8ab03c02b7 [hotfix][build] Remove unused
>>>> akka-testkit dependencies
>>>> dd34b050e8e7bd4b03ad0870a432b1631e1c0e9d [hotfix][build] Remove unused
>>>> shaded-asm7 dependencies
>>>>
>>>> // dependency changes
>>>> 244d2db78307cd7dff1c60a664046adb6fe5c405 [hotfix][web][build] Cleanup
>>>> dependencies
>>>>
>>>> In my opinion, everything that is not a typo, a compile error (breaking
>>> the
>>>> master) or something generated (like parts of the docs) should go
>>> through a
>>>> quick pull request.
>>>> Why? I don't think many people review changes in the commit log in a way
>>>> they review pull request changes.
>>>>
>>>> In addition to that, I propose to prefix hotfixes that have been added as
>>>> part of a ticket with that ticket number.
>>>> So instead of "[hotfix] Harden kubernetes test", we do
>>>> "[FLINK-13978][minor]
>>>> Harden kubernetes test".
>>>> Why? For people checking the commit history, it is much easier to see if
>>> a
>>>> hotfix has been reviewed as part of a JIRA ticket review, or whether it
>>> is
>>>> a "hotpush" hotfix.
>>>>
>>>> For changes that are too small for a JIRA ticket, but need a review, I
>>>> propose to use the "[minor]" tag. A good example of such a change is
>>> this:
>>>>
>>>>
>>> https://github.com/apache/flink/commit/0dc4e767c9c48ac58430a59d05185f2b071f53f5
>>>>
>>>> My tagging minor changes accordingly in the pull requests, it is also
>>>> easier for fellow committers to quickly check them.
>>>>
>>>> Summary:
>>>> [FLINK-XXXX]: regular, reviewed change
>>>> [FLINK-XXXX][minor]: minor, unrelated changes reviewed with a regular
>>>> ticket
>>>> [minor]: minor, reviewed change
>>>> [hotfix]: unreviewed change that fixes a typo, compile error or something
>>>> generated
>>>>
>>>>
>>>> What's your opinion on this?
>>>>
>>>>
>>>> On Sat, May 28, 2016 at 1:36 PM Vasiliki Kalavri <
>>>> vasilikikalavri@gmail.com>
>>>> wrote:
>>>>
>>>>> Hi all,
>>>>>
>>>>> in principle I agree with Max. I personally avoid hotfixes and always
>>>> open
>>>>> a PR, even for javadoc improvements.
>>>>>
>>>>> I believe the main problem is that we don't have a clear definition of
>>>> what
>>>>> constitutes a "hotfix". Ideally, even cosmetic changes and
>>> documentation
>>>>> should be reviewed; I've seen documentation added as a hotfix that had
>>>>> spelling mistakes, which led to another hotfix... Using hotfixes to do
>>>>> major refactoring or add features is absolutely unacceptable, in my
>>> view.
>>>>> On the other hand, with the current PR load it's not practical to ban
>>>>> hotfixes all together.
>>>>>
>>>>> I would suggest to update our contribution guidelines with some
>>>> definition
>>>>> of a hotfix. We could add a list of questions to ask before pushing
>>> one.
>>>>> e.g.:
>>>>> - does the change fix a spelling mistake in the docs? => hotfix
>>>>> - does the change add a missing javadoc? => hotfix
>>>>> - does the change improve a comment? => hotfix?
>>>>> - is the change a small refactoring in a code component you are
>>>> maintainer
>>>>> of? => hotfix
>>>>> - did you change code in a component you are not very familiar with /
>>> not
>>>>> the maintainer of? => open PR
>>>>> - is this major refactoring? (e.g. more than X lines of code) => open
>>> PR
>>>>> - does it fix a trivial bug? => open JIRA and PR
>>>>>
>>>>> and so on...
>>>>>
>>>>> What do you think?
>>>>>
>>>>> Cheers,
>>>>> -V.
>>>>>
>>>>> On 27 May 2016 at 17:40, Greg Hogan <co...@greghogan.com> wrote:
>>>>>
>>>>>> Max,
>>>>>>
>>>>>> I certainly agree that hotfixes are not ideal for large refactorings
>>>> and
>>>>>> new features. Some thoughts ...
>>>>>>
>>>>>> A hotfix should be maven verified, as should a rebased PR. Travis is
>>>>> often
>>>>>> backed up for half a day or more.
>>>>>>
>>>>>> Is our Jira and PR process sufficiently agile to handle these
>>> hotfixes?
>>>>>> Will committers simply include hotfixes with other PRs, and would it
>>> be
>>>>>> better to retain these as smaller, separate commits?
>>>>>>
>>>>>> For these cosmetic changes and small updates will the Jira and PR
>>> yield
>>>>>> beneficial documentation addition to what is provided in the commit
>>>>>> message?
>>>>>>
>>>>>> Counting hotfixes by contributor, the top of the list looks as I
>>> would
>>>>>> expect.
>>>>>>
>>>>>> Greg
>>>>>>
>>>>>> Note: this summary is rather naive and includes non-squashed hotfix
>>>>> commits
>>>>>> included in a PR
>>>>>> $ git shortlog --grep 'hotfix' -s -n release-0.9.0..
>>>>>> 94 Stephan Ewen
>>>>>> 42 Aljoscha Krettek
>>>>>> 20 Till Rohrmann
>>>>>> 16 Robert Metzger
>>>>>> 13 Ufuk Celebi
>>>>>> 9 Fabian Hueske
>>>>>> 9 Maximilian Michels
>>>>>> 6 Greg Hogan
>>>>>> 5 Stefano Baghino
>>>>>> 3 smarthi
>>>>>> 2 Andrea Sella
>>>>>> 2 Gyula Fora
>>>>>> 2 Jun Aoki
>>>>>> 2 Sachin Goel
>>>>>> 2 mjsax
>>>>>> 2 zentol
>>>>>> 1 Alexander Alexandrov
>>>>>> 1 Gabor Gevay
>>>>>> 1 Prez Cannady
>>>>>> 1 Steve Cosenza
>>>>>> 1 Suminda Dharmasena
>>>>>> 1 chengxiang li
>>>>>> 1 jaoki
>>>>>> 1 kl0u
>>>>>> 1 qingmeng.wyh
>>>>>> 1 sksamuel
>>>>>> 1 vasia
>>>>>>
>>>>>> On Fri, May 27, 2016 at 6:10 AM, Maximilian Michels <mx...@apache.org>
>>>>>> wrote:
>>>>>>
>>>>>>> Hi Flinksters,
>>>>>>>
>>>>>>> I'd like to address an issue that has been concerning me for a
>>> while.
>>>>>>> In the Flink community we like to push "hotfixes" to the master.
>>>>>>> Hotfixes come in various shapes: From very small cosmetic changes
>>>>>>> (JavaDoc) to major refactoring and even new features.
>>>>>>>
>>>>>>> IMHO we should move away from these hotfixes. Here's why:
>>>>>>>
>>>>>>> 1) They tend to break the master because they lack test coverage
>>>>>>> 2) They are usually not communicated with the maintainer or person
>>>>>>> working on the part being fixed
>>>>>>> 3) They are not properly documented for future reference or
>>>> follow-ups
>>>>>>> (JIRA/Github)
>>>>>>>
>>>>>>> That's why I have chosen not to push hotfixes anymore. Even for
>>> small
>>>>>>> fixes, I'll open a JIRA/Github issue. The only exception might be
>>>>>>> fixing a comment. It improves communication, documentation, and
>>> test
>>>>>>> coverage. All this helps to mature Flink and develop the community
>>> in
>>>>>>> a transparent way.
>>>>>>>
>>>>>>> I'm not sure what our contribution guidelines say about this but I
>>>>>>> would like to update them to explicitly address hotfixes. Let me
>>> know
>>>>>>> what you think.
>>>>>>>
>>>>>>> Best,
>>>>>>> Max
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
Re: Hotfixes on the master
Posted by Aljoscha Krettek <al...@apache.org>.
Side comment: the problem of not knowing where a commit came from could
be fixed by always including merge commits, just saying... ;-)
Aljoscha
On 19.02.20 11:43, Robert Metzger wrote:
> Thank you for your response. Your response makes me realize that I should
> have first asked whether other committers consider the amount of hotfix
> commits problematic or not.
> From the number of responses to my message, I have the feeling that most
> committers are not concerned.
>
> I personally believe that the amount of hotfix commits is too high. There
> have been a few cases of hotfixes committed recently that have been
> commented by other committers, have been reverted quickly after or amended
> in another hotfix. My hope is that a proper PR review would have caught
> some of those cases.
> Additionally, there's almost no papertrail around hotfixes.
> Regular commits have a JIRA ticket, which usually has some problem
> statement and ideally some discussion, and a pull request review.
> Since we are using the Merge button in PRs quite frequently, we also don't
> have the "This closes #12345" message in the commit anymore, which would at
> least point to the review PR ... so when I'm checking the commit history, I
> need to manually search the PR history as well, to see if there is more
> context on a commit in a PR review or not. (Hence my minor vs hotfix
> proposal)
>
>
> I'm happy to leave things as-is for now. As long as there are still people
> catching mistakes made in hotfix commits, I don't see a decline in quality.
>
>
>
>
> On Tue, Feb 18, 2020 at 7:58 PM Till Rohrmann <tr...@apache.org> wrote:
>
>> Thanks for raising this point Robert. I think it is important to remind the
>> community about the agreed practices once in a while.
>>
>> In most of the cases I had the impression that the majority of the
>> community sticks to the agreed rules. W/o more detailed numbers (how many
>> of the hotfix commits are of category b) I think this discussion makes only
>> limited sense. Moreover, what is the underlying problem you would like to
>> solve here? Is it that too many commits have the hotfix tag in the commit
>> log? Is it that it's hard to figure out with which PR a hotfix commit has
>> been merged? Or did you observe a decline in Flink's quality because of too
>> many unreviewed changes? If we are discussing the latter case, then I think
>> it is very urgent. In the former cases I'm not entirely sure whether this
>> is an immediate problem because from what I have seen people include many
>> more clean up/hotfix commits in their PRs these days.
>>
>> Concerning the proposed practice with the [minor] tag I'm a bit torn. The
>> benefit of not doing it like this is that it's easier to see which commits
>> need to be cherry-picked if a feature needs to be ported, for example. On
>> the other hand if the commits are not unrelated and the feature commits use
>> parts of the hotfix commits, then it makes the cherry-picking more tricky
>> and the commits should have the JIRA issue tag. But I would be fine with
>> trying it out.
>>
>> Cheers,
>> Till
>>
>> On Mon, Feb 17, 2020 at 3:56 PM Robert Metzger <rm...@apache.org>
>> wrote:
>>
>>> Hi all,
>>>
>>> I would like to revive this very old thread on the topic of "unreviewed
>>> hotfixes on master" again.
>>> Out of the 35 commits listed on the latest commits page on GitHub, 18
>> have
>>> the tag "hotfix", on the next page it is 9, then 16, 17, ...
>>> In the last 140 commits, 42% were hotfixes.
>>>
>>> For the sake of this discussion, let's distinguish between two types of
>>> hotfixes:
>>> a) *reviewed hotfix commits*: They have been reviewed through a pull
>>> request, then committed to master.
>>> b) *unreviewed hotfix commits*: These have been pushed straight to
>> master,
>>> without a review.
>>>
>>> It's quite difficult to find out whether a hotfix has been reviewed or
>> not
>>> (because many hotfix commits are reviewed & pushed as part of a PR), but
>>> here are some recent examples of commits where I could not find evidence
>> of
>>> a pull request:
>>>
>>> // these could probably be combined into on JIRA ticket, as they affect
>> the
>>> same component + they touch dependencies
>>> 47a1725ae14a772ba8590ee97dffd7fdf5bc04b2 [hotfix][docs][conf] Log
>> included
>>> packages / excluded classes
>>> a5894677d95336a67d5539584b9204bcdd14fac5 [hotfix][docs][conf] Setup
>> logging
>>> for generator
>>> 325927064542c2d018f9da33660c1cdf57e0e382 [hotfix][docs][conf] Add query
>>> service port to port section
>>> 3c696a34145e838c046805b36553a50ec9bfbda0 [hotfix][docs][conf] Add query
>>> service port to port section
>>>
>>> // dependency change
>>> 736ebc0b40abab88902ada3f564777c3ade03001 [hotfix][build] Remove various
>>> unused test dependencies
>>>
>>> // more than a regeneration / typo / compile error change
>>> 30b5f6173e688ea20b82226db6923db19dec29a5 [hotfix][tests] Adjust
>>> FileUtilsTest.testDeleteSymbolicLinkDirectory() to handle unsupported
>>> situations in Windows
>>> fc59aa4ecc2a7170bfda14ffadf0a30aa2b793bf [FLINK-16065][core] Unignore
>>> FileUtilsTest.testDeleteDirectoryConcurrently()
>>>
>>> // dependency changes
>>> fe7145787a7f36b21aad748ffea4ee8ab03c02b7 [hotfix][build] Remove unused
>>> akka-testkit dependencies
>>> dd34b050e8e7bd4b03ad0870a432b1631e1c0e9d [hotfix][build] Remove unused
>>> shaded-asm7 dependencies
>>>
>>> // dependency changes
>>> 244d2db78307cd7dff1c60a664046adb6fe5c405 [hotfix][web][build] Cleanup
>>> dependencies
>>>
>>> In my opinion, everything that is not a typo, a compile error (breaking
>> the
>>> master) or something generated (like parts of the docs) should go
>> through a
>>> quick pull request.
>>> Why? I don't think many people review changes in the commit log in a way
>>> they review pull request changes.
>>>
>>> In addition to that, I propose to prefix hotfixes that have been added as
>>> part of a ticket with that ticket number.
>>> So instead of "[hotfix] Harden kubernetes test", we do
>>> "[FLINK-13978][minor]
>>> Harden kubernetes test".
>>> Why? For people checking the commit history, it is much easier to see if
>> a
>>> hotfix has been reviewed as part of a JIRA ticket review, or whether it
>> is
>>> a "hotpush" hotfix.
>>>
>>> For changes that are too small for a JIRA ticket, but need a review, I
>>> propose to use the "[minor]" tag. A good example of such a change is
>> this:
>>>
>>>
>> https://github.com/apache/flink/commit/0dc4e767c9c48ac58430a59d05185f2b071f53f5
>>>
>>> My tagging minor changes accordingly in the pull requests, it is also
>>> easier for fellow committers to quickly check them.
>>>
>>> Summary:
>>> [FLINK-XXXX]: regular, reviewed change
>>> [FLINK-XXXX][minor]: minor, unrelated changes reviewed with a regular
>>> ticket
>>> [minor]: minor, reviewed change
>>> [hotfix]: unreviewed change that fixes a typo, compile error or something
>>> generated
>>>
>>>
>>> What's your opinion on this?
>>>
>>>
>>> On Sat, May 28, 2016 at 1:36 PM Vasiliki Kalavri <
>>> vasilikikalavri@gmail.com>
>>> wrote:
>>>
>>>> Hi all,
>>>>
>>>> in principle I agree with Max. I personally avoid hotfixes and always
>>> open
>>>> a PR, even for javadoc improvements.
>>>>
>>>> I believe the main problem is that we don't have a clear definition of
>>> what
>>>> constitutes a "hotfix". Ideally, even cosmetic changes and
>> documentation
>>>> should be reviewed; I've seen documentation added as a hotfix that had
>>>> spelling mistakes, which led to another hotfix... Using hotfixes to do
>>>> major refactoring or add features is absolutely unacceptable, in my
>> view.
>>>> On the other hand, with the current PR load it's not practical to ban
>>>> hotfixes all together.
>>>>
>>>> I would suggest to update our contribution guidelines with some
>>> definition
>>>> of a hotfix. We could add a list of questions to ask before pushing
>> one.
>>>> e.g.:
>>>> - does the change fix a spelling mistake in the docs? => hotfix
>>>> - does the change add a missing javadoc? => hotfix
>>>> - does the change improve a comment? => hotfix?
>>>> - is the change a small refactoring in a code component you are
>>> maintainer
>>>> of? => hotfix
>>>> - did you change code in a component you are not very familiar with /
>> not
>>>> the maintainer of? => open PR
>>>> - is this major refactoring? (e.g. more than X lines of code) => open
>> PR
>>>> - does it fix a trivial bug? => open JIRA and PR
>>>>
>>>> and so on...
>>>>
>>>> What do you think?
>>>>
>>>> Cheers,
>>>> -V.
>>>>
>>>> On 27 May 2016 at 17:40, Greg Hogan <co...@greghogan.com> wrote:
>>>>
>>>>> Max,
>>>>>
>>>>> I certainly agree that hotfixes are not ideal for large refactorings
>>> and
>>>>> new features. Some thoughts ...
>>>>>
>>>>> A hotfix should be maven verified, as should a rebased PR. Travis is
>>>> often
>>>>> backed up for half a day or more.
>>>>>
>>>>> Is our Jira and PR process sufficiently agile to handle these
>> hotfixes?
>>>>> Will committers simply include hotfixes with other PRs, and would it
>> be
>>>>> better to retain these as smaller, separate commits?
>>>>>
>>>>> For these cosmetic changes and small updates will the Jira and PR
>> yield
>>>>> beneficial documentation addition to what is provided in the commit
>>>>> message?
>>>>>
>>>>> Counting hotfixes by contributor, the top of the list looks as I
>> would
>>>>> expect.
>>>>>
>>>>> Greg
>>>>>
>>>>> Note: this summary is rather naive and includes non-squashed hotfix
>>>> commits
>>>>> included in a PR
>>>>> $ git shortlog --grep 'hotfix' -s -n release-0.9.0..
>>>>> 94 Stephan Ewen
>>>>> 42 Aljoscha Krettek
>>>>> 20 Till Rohrmann
>>>>> 16 Robert Metzger
>>>>> 13 Ufuk Celebi
>>>>> 9 Fabian Hueske
>>>>> 9 Maximilian Michels
>>>>> 6 Greg Hogan
>>>>> 5 Stefano Baghino
>>>>> 3 smarthi
>>>>> 2 Andrea Sella
>>>>> 2 Gyula Fora
>>>>> 2 Jun Aoki
>>>>> 2 Sachin Goel
>>>>> 2 mjsax
>>>>> 2 zentol
>>>>> 1 Alexander Alexandrov
>>>>> 1 Gabor Gevay
>>>>> 1 Prez Cannady
>>>>> 1 Steve Cosenza
>>>>> 1 Suminda Dharmasena
>>>>> 1 chengxiang li
>>>>> 1 jaoki
>>>>> 1 kl0u
>>>>> 1 qingmeng.wyh
>>>>> 1 sksamuel
>>>>> 1 vasia
>>>>>
>>>>> On Fri, May 27, 2016 at 6:10 AM, Maximilian Michels <mx...@apache.org>
>>>>> wrote:
>>>>>
>>>>>> Hi Flinksters,
>>>>>>
>>>>>> I'd like to address an issue that has been concerning me for a
>> while.
>>>>>> In the Flink community we like to push "hotfixes" to the master.
>>>>>> Hotfixes come in various shapes: From very small cosmetic changes
>>>>>> (JavaDoc) to major refactoring and even new features.
>>>>>>
>>>>>> IMHO we should move away from these hotfixes. Here's why:
>>>>>>
>>>>>> 1) They tend to break the master because they lack test coverage
>>>>>> 2) They are usually not communicated with the maintainer or person
>>>>>> working on the part being fixed
>>>>>> 3) They are not properly documented for future reference or
>>> follow-ups
>>>>>> (JIRA/Github)
>>>>>>
>>>>>> That's why I have chosen not to push hotfixes anymore. Even for
>> small
>>>>>> fixes, I'll open a JIRA/Github issue. The only exception might be
>>>>>> fixing a comment. It improves communication, documentation, and
>> test
>>>>>> coverage. All this helps to mature Flink and develop the community
>> in
>>>>>> a transparent way.
>>>>>>
>>>>>> I'm not sure what our contribution guidelines say about this but I
>>>>>> would like to update them to explicitly address hotfixes. Let me
>> know
>>>>>> what you think.
>>>>>>
>>>>>> Best,
>>>>>> Max
>>>>>>
>>>>>
>>>>
>>>
>>
>
Re: Hotfixes on the master
Posted by Robert Metzger <rm...@apache.org>.
Thank you for your response. Your response makes me realize that I should
have first asked whether other committers consider the amount of hotfix
commits problematic or not.
From the number of responses to my message, I have the feeling that most
committers are not concerned.
I personally believe that the amount of hotfix commits is too high. There
have been a few cases of hotfixes committed recently that have been
commented by other committers, have been reverted quickly after or amended
in another hotfix. My hope is that a proper PR review would have caught
some of those cases.
Additionally, there's almost no papertrail around hotfixes.
Regular commits have a JIRA ticket, which usually has some problem
statement and ideally some discussion, and a pull request review.
Since we are using the Merge button in PRs quite frequently, we also don't
have the "This closes #12345" message in the commit anymore, which would at
least point to the review PR ... so when I'm checking the commit history, I
need to manually search the PR history as well, to see if there is more
context on a commit in a PR review or not. (Hence my minor vs hotfix
proposal)
I'm happy to leave things as-is for now. As long as there are still people
catching mistakes made in hotfix commits, I don't see a decline in quality.
On Tue, Feb 18, 2020 at 7:58 PM Till Rohrmann <tr...@apache.org> wrote:
> Thanks for raising this point Robert. I think it is important to remind the
> community about the agreed practices once in a while.
>
> In most of the cases I had the impression that the majority of the
> community sticks to the agreed rules. W/o more detailed numbers (how many
> of the hotfix commits are of category b) I think this discussion makes only
> limited sense. Moreover, what is the underlying problem you would like to
> solve here? Is it that too many commits have the hotfix tag in the commit
> log? Is it that it's hard to figure out with which PR a hotfix commit has
> been merged? Or did you observe a decline in Flink's quality because of too
> many unreviewed changes? If we are discussing the latter case, then I think
> it is very urgent. In the former cases I'm not entirely sure whether this
> is an immediate problem because from what I have seen people include many
> more clean up/hotfix commits in their PRs these days.
>
> Concerning the proposed practice with the [minor] tag I'm a bit torn. The
> benefit of not doing it like this is that it's easier to see which commits
> need to be cherry-picked if a feature needs to be ported, for example. On
> the other hand if the commits are not unrelated and the feature commits use
> parts of the hotfix commits, then it makes the cherry-picking more tricky
> and the commits should have the JIRA issue tag. But I would be fine with
> trying it out.
>
> Cheers,
> Till
>
> On Mon, Feb 17, 2020 at 3:56 PM Robert Metzger <rm...@apache.org>
> wrote:
>
> > Hi all,
> >
> > I would like to revive this very old thread on the topic of "unreviewed
> > hotfixes on master" again.
> > Out of the 35 commits listed on the latest commits page on GitHub, 18
> have
> > the tag "hotfix", on the next page it is 9, then 16, 17, ...
> > In the last 140 commits, 42% were hotfixes.
> >
> > For the sake of this discussion, let's distinguish between two types of
> > hotfixes:
> > a) *reviewed hotfix commits*: They have been reviewed through a pull
> > request, then committed to master.
> > b) *unreviewed hotfix commits*: These have been pushed straight to
> master,
> > without a review.
> >
> > It's quite difficult to find out whether a hotfix has been reviewed or
> not
> > (because many hotfix commits are reviewed & pushed as part of a PR), but
> > here are some recent examples of commits where I could not find evidence
> of
> > a pull request:
> >
> > // these could probably be combined into on JIRA ticket, as they affect
> the
> > same component + they touch dependencies
> > 47a1725ae14a772ba8590ee97dffd7fdf5bc04b2 [hotfix][docs][conf] Log
> included
> > packages / excluded classes
> > a5894677d95336a67d5539584b9204bcdd14fac5 [hotfix][docs][conf] Setup
> logging
> > for generator
> > 325927064542c2d018f9da33660c1cdf57e0e382 [hotfix][docs][conf] Add query
> > service port to port section
> > 3c696a34145e838c046805b36553a50ec9bfbda0 [hotfix][docs][conf] Add query
> > service port to port section
> >
> > // dependency change
> > 736ebc0b40abab88902ada3f564777c3ade03001 [hotfix][build] Remove various
> > unused test dependencies
> >
> > // more than a regeneration / typo / compile error change
> > 30b5f6173e688ea20b82226db6923db19dec29a5 [hotfix][tests] Adjust
> > FileUtilsTest.testDeleteSymbolicLinkDirectory() to handle unsupported
> > situations in Windows
> > fc59aa4ecc2a7170bfda14ffadf0a30aa2b793bf [FLINK-16065][core] Unignore
> > FileUtilsTest.testDeleteDirectoryConcurrently()
> >
> > // dependency changes
> > fe7145787a7f36b21aad748ffea4ee8ab03c02b7 [hotfix][build] Remove unused
> > akka-testkit dependencies
> > dd34b050e8e7bd4b03ad0870a432b1631e1c0e9d [hotfix][build] Remove unused
> > shaded-asm7 dependencies
> >
> > // dependency changes
> > 244d2db78307cd7dff1c60a664046adb6fe5c405 [hotfix][web][build] Cleanup
> > dependencies
> >
> > In my opinion, everything that is not a typo, a compile error (breaking
> the
> > master) or something generated (like parts of the docs) should go
> through a
> > quick pull request.
> > Why? I don't think many people review changes in the commit log in a way
> > they review pull request changes.
> >
> > In addition to that, I propose to prefix hotfixes that have been added as
> > part of a ticket with that ticket number.
> > So instead of "[hotfix] Harden kubernetes test", we do
> > "[FLINK-13978][minor]
> > Harden kubernetes test".
> > Why? For people checking the commit history, it is much easier to see if
> a
> > hotfix has been reviewed as part of a JIRA ticket review, or whether it
> is
> > a "hotpush" hotfix.
> >
> > For changes that are too small for a JIRA ticket, but need a review, I
> > propose to use the "[minor]" tag. A good example of such a change is
> this:
> >
> >
> https://github.com/apache/flink/commit/0dc4e767c9c48ac58430a59d05185f2b071f53f5
> >
> > My tagging minor changes accordingly in the pull requests, it is also
> > easier for fellow committers to quickly check them.
> >
> > Summary:
> > [FLINK-XXXX]: regular, reviewed change
> > [FLINK-XXXX][minor]: minor, unrelated changes reviewed with a regular
> > ticket
> > [minor]: minor, reviewed change
> > [hotfix]: unreviewed change that fixes a typo, compile error or something
> > generated
> >
> >
> > What's your opinion on this?
> >
> >
> > On Sat, May 28, 2016 at 1:36 PM Vasiliki Kalavri <
> > vasilikikalavri@gmail.com>
> > wrote:
> >
> > > Hi all,
> > >
> > > in principle I agree with Max. I personally avoid hotfixes and always
> > open
> > > a PR, even for javadoc improvements.
> > >
> > > I believe the main problem is that we don't have a clear definition of
> > what
> > > constitutes a "hotfix". Ideally, even cosmetic changes and
> documentation
> > > should be reviewed; I've seen documentation added as a hotfix that had
> > > spelling mistakes, which led to another hotfix... Using hotfixes to do
> > > major refactoring or add features is absolutely unacceptable, in my
> view.
> > > On the other hand, with the current PR load it's not practical to ban
> > > hotfixes all together.
> > >
> > > I would suggest to update our contribution guidelines with some
> > definition
> > > of a hotfix. We could add a list of questions to ask before pushing
> one.
> > > e.g.:
> > > - does the change fix a spelling mistake in the docs? => hotfix
> > > - does the change add a missing javadoc? => hotfix
> > > - does the change improve a comment? => hotfix?
> > > - is the change a small refactoring in a code component you are
> > maintainer
> > > of? => hotfix
> > > - did you change code in a component you are not very familiar with /
> not
> > > the maintainer of? => open PR
> > > - is this major refactoring? (e.g. more than X lines of code) => open
> PR
> > > - does it fix a trivial bug? => open JIRA and PR
> > >
> > > and so on...
> > >
> > > What do you think?
> > >
> > > Cheers,
> > > -V.
> > >
> > > On 27 May 2016 at 17:40, Greg Hogan <co...@greghogan.com> wrote:
> > >
> > > > Max,
> > > >
> > > > I certainly agree that hotfixes are not ideal for large refactorings
> > and
> > > > new features. Some thoughts ...
> > > >
> > > > A hotfix should be maven verified, as should a rebased PR. Travis is
> > > often
> > > > backed up for half a day or more.
> > > >
> > > > Is our Jira and PR process sufficiently agile to handle these
> hotfixes?
> > > > Will committers simply include hotfixes with other PRs, and would it
> be
> > > > better to retain these as smaller, separate commits?
> > > >
> > > > For these cosmetic changes and small updates will the Jira and PR
> yield
> > > > beneficial documentation addition to what is provided in the commit
> > > > message?
> > > >
> > > > Counting hotfixes by contributor, the top of the list looks as I
> would
> > > > expect.
> > > >
> > > > Greg
> > > >
> > > > Note: this summary is rather naive and includes non-squashed hotfix
> > > commits
> > > > included in a PR
> > > > $ git shortlog --grep 'hotfix' -s -n release-0.9.0..
> > > > 94 Stephan Ewen
> > > > 42 Aljoscha Krettek
> > > > 20 Till Rohrmann
> > > > 16 Robert Metzger
> > > > 13 Ufuk Celebi
> > > > 9 Fabian Hueske
> > > > 9 Maximilian Michels
> > > > 6 Greg Hogan
> > > > 5 Stefano Baghino
> > > > 3 smarthi
> > > > 2 Andrea Sella
> > > > 2 Gyula Fora
> > > > 2 Jun Aoki
> > > > 2 Sachin Goel
> > > > 2 mjsax
> > > > 2 zentol
> > > > 1 Alexander Alexandrov
> > > > 1 Gabor Gevay
> > > > 1 Prez Cannady
> > > > 1 Steve Cosenza
> > > > 1 Suminda Dharmasena
> > > > 1 chengxiang li
> > > > 1 jaoki
> > > > 1 kl0u
> > > > 1 qingmeng.wyh
> > > > 1 sksamuel
> > > > 1 vasia
> > > >
> > > > On Fri, May 27, 2016 at 6:10 AM, Maximilian Michels <mx...@apache.org>
> > > > wrote:
> > > >
> > > > > Hi Flinksters,
> > > > >
> > > > > I'd like to address an issue that has been concerning me for a
> while.
> > > > > In the Flink community we like to push "hotfixes" to the master.
> > > > > Hotfixes come in various shapes: From very small cosmetic changes
> > > > > (JavaDoc) to major refactoring and even new features.
> > > > >
> > > > > IMHO we should move away from these hotfixes. Here's why:
> > > > >
> > > > > 1) They tend to break the master because they lack test coverage
> > > > > 2) They are usually not communicated with the maintainer or person
> > > > > working on the part being fixed
> > > > > 3) They are not properly documented for future reference or
> > follow-ups
> > > > > (JIRA/Github)
> > > > >
> > > > > That's why I have chosen not to push hotfixes anymore. Even for
> small
> > > > > fixes, I'll open a JIRA/Github issue. The only exception might be
> > > > > fixing a comment. It improves communication, documentation, and
> test
> > > > > coverage. All this helps to mature Flink and develop the community
> in
> > > > > a transparent way.
> > > > >
> > > > > I'm not sure what our contribution guidelines say about this but I
> > > > > would like to update them to explicitly address hotfixes. Let me
> know
> > > > > what you think.
> > > > >
> > > > > Best,
> > > > > Max
> > > > >
> > > >
> > >
> >
>
Re: Hotfixes on the master
Posted by Till Rohrmann <tr...@apache.org>.
Thanks for raising this point Robert. I think it is important to remind the
community about the agreed practices once in a while.
In most of the cases I had the impression that the majority of the
community sticks to the agreed rules. W/o more detailed numbers (how many
of the hotfix commits are of category b) I think this discussion makes only
limited sense. Moreover, what is the underlying problem you would like to
solve here? Is it that too many commits have the hotfix tag in the commit
log? Is it that it's hard to figure out with which PR a hotfix commit has
been merged? Or did you observe a decline in Flink's quality because of too
many unreviewed changes? If we are discussing the latter case, then I think
it is very urgent. In the former cases I'm not entirely sure whether this
is an immediate problem because from what I have seen people include many
more clean up/hotfix commits in their PRs these days.
Concerning the proposed practice with the [minor] tag I'm a bit torn. The
benefit of not doing it like this is that it's easier to see which commits
need to be cherry-picked if a feature needs to be ported, for example. On
the other hand if the commits are not unrelated and the feature commits use
parts of the hotfix commits, then it makes the cherry-picking more tricky
and the commits should have the JIRA issue tag. But I would be fine with
trying it out.
Cheers,
Till
On Mon, Feb 17, 2020 at 3:56 PM Robert Metzger <rm...@apache.org> wrote:
> Hi all,
>
> I would like to revive this very old thread on the topic of "unreviewed
> hotfixes on master" again.
> Out of the 35 commits listed on the latest commits page on GitHub, 18 have
> the tag "hotfix", on the next page it is 9, then 16, 17, ...
> In the last 140 commits, 42% were hotfixes.
>
> For the sake of this discussion, let's distinguish between two types of
> hotfixes:
> a) *reviewed hotfix commits*: They have been reviewed through a pull
> request, then committed to master.
> b) *unreviewed hotfix commits*: These have been pushed straight to master,
> without a review.
>
> It's quite difficult to find out whether a hotfix has been reviewed or not
> (because many hotfix commits are reviewed & pushed as part of a PR), but
> here are some recent examples of commits where I could not find evidence of
> a pull request:
>
> // these could probably be combined into on JIRA ticket, as they affect the
> same component + they touch dependencies
> 47a1725ae14a772ba8590ee97dffd7fdf5bc04b2 [hotfix][docs][conf] Log included
> packages / excluded classes
> a5894677d95336a67d5539584b9204bcdd14fac5 [hotfix][docs][conf] Setup logging
> for generator
> 325927064542c2d018f9da33660c1cdf57e0e382 [hotfix][docs][conf] Add query
> service port to port section
> 3c696a34145e838c046805b36553a50ec9bfbda0 [hotfix][docs][conf] Add query
> service port to port section
>
> // dependency change
> 736ebc0b40abab88902ada3f564777c3ade03001 [hotfix][build] Remove various
> unused test dependencies
>
> // more than a regeneration / typo / compile error change
> 30b5f6173e688ea20b82226db6923db19dec29a5 [hotfix][tests] Adjust
> FileUtilsTest.testDeleteSymbolicLinkDirectory() to handle unsupported
> situations in Windows
> fc59aa4ecc2a7170bfda14ffadf0a30aa2b793bf [FLINK-16065][core] Unignore
> FileUtilsTest.testDeleteDirectoryConcurrently()
>
> // dependency changes
> fe7145787a7f36b21aad748ffea4ee8ab03c02b7 [hotfix][build] Remove unused
> akka-testkit dependencies
> dd34b050e8e7bd4b03ad0870a432b1631e1c0e9d [hotfix][build] Remove unused
> shaded-asm7 dependencies
>
> // dependency changes
> 244d2db78307cd7dff1c60a664046adb6fe5c405 [hotfix][web][build] Cleanup
> dependencies
>
> In my opinion, everything that is not a typo, a compile error (breaking the
> master) or something generated (like parts of the docs) should go through a
> quick pull request.
> Why? I don't think many people review changes in the commit log in a way
> they review pull request changes.
>
> In addition to that, I propose to prefix hotfixes that have been added as
> part of a ticket with that ticket number.
> So instead of "[hotfix] Harden kubernetes test", we do
> "[FLINK-13978][minor]
> Harden kubernetes test".
> Why? For people checking the commit history, it is much easier to see if a
> hotfix has been reviewed as part of a JIRA ticket review, or whether it is
> a "hotpush" hotfix.
>
> For changes that are too small for a JIRA ticket, but need a review, I
> propose to use the "[minor]" tag. A good example of such a change is this:
>
> https://github.com/apache/flink/commit/0dc4e767c9c48ac58430a59d05185f2b071f53f5
>
> My tagging minor changes accordingly in the pull requests, it is also
> easier for fellow committers to quickly check them.
>
> Summary:
> [FLINK-XXXX]: regular, reviewed change
> [FLINK-XXXX][minor]: minor, unrelated changes reviewed with a regular
> ticket
> [minor]: minor, reviewed change
> [hotfix]: unreviewed change that fixes a typo, compile error or something
> generated
>
>
> What's your opinion on this?
>
>
> On Sat, May 28, 2016 at 1:36 PM Vasiliki Kalavri <
> vasilikikalavri@gmail.com>
> wrote:
>
> > Hi all,
> >
> > in principle I agree with Max. I personally avoid hotfixes and always
> open
> > a PR, even for javadoc improvements.
> >
> > I believe the main problem is that we don't have a clear definition of
> what
> > constitutes a "hotfix". Ideally, even cosmetic changes and documentation
> > should be reviewed; I've seen documentation added as a hotfix that had
> > spelling mistakes, which led to another hotfix... Using hotfixes to do
> > major refactoring or add features is absolutely unacceptable, in my view.
> > On the other hand, with the current PR load it's not practical to ban
> > hotfixes all together.
> >
> > I would suggest to update our contribution guidelines with some
> definition
> > of a hotfix. We could add a list of questions to ask before pushing one.
> > e.g.:
> > - does the change fix a spelling mistake in the docs? => hotfix
> > - does the change add a missing javadoc? => hotfix
> > - does the change improve a comment? => hotfix?
> > - is the change a small refactoring in a code component you are
> maintainer
> > of? => hotfix
> > - did you change code in a component you are not very familiar with / not
> > the maintainer of? => open PR
> > - is this major refactoring? (e.g. more than X lines of code) => open PR
> > - does it fix a trivial bug? => open JIRA and PR
> >
> > and so on...
> >
> > What do you think?
> >
> > Cheers,
> > -V.
> >
> > On 27 May 2016 at 17:40, Greg Hogan <co...@greghogan.com> wrote:
> >
> > > Max,
> > >
> > > I certainly agree that hotfixes are not ideal for large refactorings
> and
> > > new features. Some thoughts ...
> > >
> > > A hotfix should be maven verified, as should a rebased PR. Travis is
> > often
> > > backed up for half a day or more.
> > >
> > > Is our Jira and PR process sufficiently agile to handle these hotfixes?
> > > Will committers simply include hotfixes with other PRs, and would it be
> > > better to retain these as smaller, separate commits?
> > >
> > > For these cosmetic changes and small updates will the Jira and PR yield
> > > beneficial documentation addition to what is provided in the commit
> > > message?
> > >
> > > Counting hotfixes by contributor, the top of the list looks as I would
> > > expect.
> > >
> > > Greg
> > >
> > > Note: this summary is rather naive and includes non-squashed hotfix
> > commits
> > > included in a PR
> > > $ git shortlog --grep 'hotfix' -s -n release-0.9.0..
> > > 94 Stephan Ewen
> > > 42 Aljoscha Krettek
> > > 20 Till Rohrmann
> > > 16 Robert Metzger
> > > 13 Ufuk Celebi
> > > 9 Fabian Hueske
> > > 9 Maximilian Michels
> > > 6 Greg Hogan
> > > 5 Stefano Baghino
> > > 3 smarthi
> > > 2 Andrea Sella
> > > 2 Gyula Fora
> > > 2 Jun Aoki
> > > 2 Sachin Goel
> > > 2 mjsax
> > > 2 zentol
> > > 1 Alexander Alexandrov
> > > 1 Gabor Gevay
> > > 1 Prez Cannady
> > > 1 Steve Cosenza
> > > 1 Suminda Dharmasena
> > > 1 chengxiang li
> > > 1 jaoki
> > > 1 kl0u
> > > 1 qingmeng.wyh
> > > 1 sksamuel
> > > 1 vasia
> > >
> > > On Fri, May 27, 2016 at 6:10 AM, Maximilian Michels <mx...@apache.org>
> > > wrote:
> > >
> > > > Hi Flinksters,
> > > >
> > > > I'd like to address an issue that has been concerning me for a while.
> > > > In the Flink community we like to push "hotfixes" to the master.
> > > > Hotfixes come in various shapes: From very small cosmetic changes
> > > > (JavaDoc) to major refactoring and even new features.
> > > >
> > > > IMHO we should move away from these hotfixes. Here's why:
> > > >
> > > > 1) They tend to break the master because they lack test coverage
> > > > 2) They are usually not communicated with the maintainer or person
> > > > working on the part being fixed
> > > > 3) They are not properly documented for future reference or
> follow-ups
> > > > (JIRA/Github)
> > > >
> > > > That's why I have chosen not to push hotfixes anymore. Even for small
> > > > fixes, I'll open a JIRA/Github issue. The only exception might be
> > > > fixing a comment. It improves communication, documentation, and test
> > > > coverage. All this helps to mature Flink and develop the community in
> > > > a transparent way.
> > > >
> > > > I'm not sure what our contribution guidelines say about this but I
> > > > would like to update them to explicitly address hotfixes. Let me know
> > > > what you think.
> > > >
> > > > Best,
> > > > Max
> > > >
> > >
> >
>