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 <trohrm...@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 <rmetz...@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 < > > vasilikikala...@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 <c...@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 <m...@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 > > > > > > > > > > > > > > >
