As the one, who does most of the “cleanups”, I also should give my take on it. Reflecting on the points made by Eric (in the original post) and Andrew, I agree with both of them.
Moving forward, I see the following options: 1. Leave it as is. Based on this discussion that shouldn’t be an option. 2. Require the backport all the way down to branch-1, including release branches. 3. Require the backport to all release lines (currently master, branch-2 and branch-1). The RMs then can decide if it should go into a release branch. If I’m not mistaken, that’s Andrews suggestion from his initial email. 4. Require the changes to be as small as possible. For the backports it should either follow 2) or 3). 5. Don’t do pure cleanup commits. Cleanups should be done as part of normal tickets if the contributor wants to do them, even if they are not directly related to the actual change. 6. Require that every cleanup ticket has a short cost-benefit analysis on it (even if it will be subjective to some extend) before moving forward (as described by Eric Badger on the original YARN ticket). For backports it then follows either 2) or 3). Taking Andrews points into account, I personally would either go with option 3) or option 6). From: Andrew Purtell <[email protected]> Reply-To: "[email protected]" <[email protected]> Date: Saturday, January 11, 2020 at 2:23 AM To: dev <[email protected]> Subject: Re: [DISCUSS] Guidelines for Code cleanup JIRAs ... and if you do commit a "cleanup" but don't backport it all the way to branch-1, then you are doing other committers a disservice, in my opinion. </rant> On Fri, Jan 10, 2020 at 5:15 PM Andrew Purtell <[email protected]<mailto:[email protected]>> wrote: I will go so far to claim "cleanup" issues can easily be a net negative given how burdensome the divergence between branches becomes as a result of them. With my PMC hat on, and one who frequently does backports, and intends to do more of them, I would like every committer to consider the value of the "cleanups" they are committing. Not every warning from a static analysis tool is worth following up on. For example: import order. Who gives a crap. (Yes yes don't introduce them!) On Fri, Jan 10, 2020 at 5:12 PM Andrew Purtell <[email protected]<mailto:[email protected]>> wrote: I'm talking about "cleanups", not introduction of new warnings. New warnings should be caught in commit and fixed before commit. It's pretty simple -- per the original post and the points I've called out -- there is a cost benefit tradeoff and not every "cleanup" issue takes that into account. I'm not calling out any specific example but I would like this to be considered going forward. Every change makes backporting a bit harder. We have three active branches: master, branch-2, branch-1. The more they diverge, the more work for everyone for every commit. "Cleanups" for their own sake do not take this into account and some warnings from static analysis tools have marginal value. On Fri, Jan 10, 2020 at 5:02 PM Nick Dimiduk <[email protected]<mailto:[email protected]>> wrote: On Fri, Jan 10, 2020 at 16:53 Andrew Purtell <[email protected]<mailto:[email protected]>> wrote: > > "Please don't run code analysis tools and then open many JIRAs that > document those findings. That activity does not put any thought into this > cost-benefit analysis." > > On this latter point, it also includes trivial checkstyle nits and low > priority findbugs findings. I’m curious why you call out these tools specifically? We have both integrated into our build. In the case of checkstyle, we control the configuration and there are plugins for both Eclipse and IntelliJ — there’s really no excuse for contributors ignoring the warnings they produce. If we don’t like some class of warning, we should adjust the rule, not ignore the check failure. I agree there’s no real value in someone coming along, running a tool, and opening a bunch of tickets. On the other hand, I very much appreciate Jan’s recent efforts to address the checkstyle issues, module by module. On Fri, Jan 10, 2020 at 4:45 PM Nick Dimiduk <[email protected]<mailto:[email protected]>> wrote: > > > Thanks for being this up Andrew. > > > > I am also +1 for code cleanup. I agree that such efforts must hit the > fork > > branches of each release line, thus: master, branch-2, branch1. > > > > I’m -0 on taking such commits to release branches. These code lines are > > should be relatively static, only receiving bug fixes for their lifetime. > > Cleanup under src/test being a notable exception to this point. > > > > -n > > > > On Fri, Jan 10, 2020 at 13:08 Sean Busbey > > <[email protected]<mailto:[email protected]>> wrote: > > > > > the link didn't work for me. here's another: > > > > > > https://s.apache.org/5yvfi > > > > > > Generally, I like this as an approach. I really value the clean up > work, > > > but cleanup / bug fixes that don't make it into earlier release lines > > then > > > make my job as an RM who does backports more difficult especially when > > they > > > touch a lot of code. I know we have too many branches, but just > handling > > > the major release lines means only 2 backports at the moment. > > > > > > I'd be happy with folks just noting a reason on the jira why something > > > couldn't go back to branch-2 or branch-1 (e.g. when something requires > > > JDK8). > > > > > > On Thu, Jan 9, 2020 at 2:12 PM Andrew Purtell > > > <[email protected]<mailto:[email protected]> > > > wrote: > > > > > > > Over on the Hadoop dev lists Eric Payne sent a great summary of > > > discussions > > > > that community has had on the tradeoffs involved with code cleanup > > > issues, > > > > and also provided an excellent set of recommendations. > > > > > > > > See the thread here: https://s.apache.org/fn5al > > > > > > > > I will include the top post below. I endorse it in its entirety as a > > > > starting point for discussion in our community as well. > > > > > > > > >>> > > > > There was some discussion on > > > > https://issues.apache.org/jira/browse/YARN-9052 > > > > about concerns surrounding the costs/benefits of code cleanup JIRAs. > > This > > > > email is to get the discussion going within a wider audience. > > > > > > > > The positive points for code cleanup JIRAs: > > > > - Clean up tech debt > > > > - Make code more readable > > > > - Make code more maintainable > > > > - Make code more performant > > > > > > > > The concerns regarding code cleanup JIRAs are as follows: > > > > - If the changes only go into trunk, then contributors and committers > > > > trying to > > > > backport to prior releases will have to create and test multiple > patch > > > > versions. > > > > - Some have voiced concerns that code cleanup JIRAs may not be tested > > as > > > > thoroughly as features and bug fixes because functionality is not > > > > supposed to > > > > change. > > > > - Any patches awaiting review that are touching the same code will > have > > > to > > > > be > > > > redone, re-tested, and re-reviewed. > > > > - JIRAs that are opened for code cleanup and not worked on right away > > > tend > > > > to > > > > clutter up the JIRA space. > > > > > > > > Here are my opinions: > > > > - Code changes of any kind force a non-trivial amount of overhead for > > > other > > > > developers. For code cleanup JIRAs, sometimes the usability, > > > > maintainability, > > > > and performance is worth the overhead. > > > > - Before opening any JIRA, please always consider whether or not the > > > added > > > > usability will outweigh the added pain you are causing other > > > developers. > > > > - If you believe the benefits outweigh the costs, please backport the > > > > changes > > > > yourself to all active lines. > > > > - Please don't run code analysis tools and then open many JIRAs that > > > > document > > > > those findings. That activity does not put any thought into this > > > > cost-benefit > > > > analysis. > > > > <<< > > > > > > > > My preference is to port all the way back to at least branch-1. Those > > > > interested in branch-1 maintenance and code lines derived from it, > like > > > > 1.3, 1.4, 1.5, and soon 1.6, can decide what to do once it lands in > > > > branch-1, but we at least need the branch-1 backport as a starting > > point > > > > addressing some of the major prerequisites: Hadoop 2 support, Java 7 > > > source > > > > level, etc. > > > > > > > > -- > > > > Best regards, > > > > Andrew > > > > > > > > Words like orphans lost among the crosstalk, meaning torn from > truth's > > > > decrepit hands > > > > - A23, Crosstalk > > > > > > > > > > > > -- > Best regards, > Andrew > > Words like orphans lost among the crosstalk, meaning torn from truth's > decrepit hands > - A23, Crosstalk > -- Best regards, Andrew Words like orphans lost among the crosstalk, meaning torn from truth's decrepit hands - A23, Crosstalk -- Best regards, Andrew Words like orphans lost among the crosstalk, meaning torn from truth's decrepit hands - A23, Crosstalk -- Best regards, Andrew Words like orphans lost among the crosstalk, meaning torn from truth's decrepit hands - A23, Crosstalk
