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 > > > > > >
