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]> wrote: > On Fri, Jan 10, 2020 at 16:53 Andrew Purtell <[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]> 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]> 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]> > > > 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
