Option three could work. I'd expect the JIRA description to include a sentence or two on the benefit. The obvious cost is the cleanup contributor/committer's time getting the change set into master, branch-2, and branch-1 - modulo differences among the code lines. The cost-benefit calculation is implicit in the committer/contributor's completion of patches for all three branches - if we have patches, they thought it worth it; otherwise, not.
On Tue, Jan 14, 2020 at 12:59 PM Jan Hentschel < [email protected]> wrote: > 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 > > -- Best regards, Andrew Words like orphans lost among the crosstalk, meaning torn from truth's decrepit hands - A23, Crosstalk
