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]> 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]> 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 > -- Best regards, Andrew Words like orphans lost among the crosstalk, meaning torn from truth's decrepit hands - A23, Crosstalk
