From dev-return-77481-archive-asf-public=cust-asf.ponee.io@hbase.apache.org Sat Jan 11 01:23:47 2020 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id E6848180657 for ; Sat, 11 Jan 2020 02:23:46 +0100 (CET) Received: (qmail 7049 invoked by uid 500); 11 Jan 2020 01:23:45 -0000 Mailing-List: contact dev-help@hbase.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@hbase.apache.org Delivered-To: mailing list dev@hbase.apache.org Received: (qmail 7037 invoked by uid 99); 11 Jan 2020 01:23:45 -0000 Received: from Unknown (HELO mailrelay1-lw-us.apache.org) (10.10.3.159) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 11 Jan 2020 01:23:45 +0000 Received: from mail-ot1-f54.google.com (mail-ot1-f54.google.com [209.85.210.54]) by mailrelay1-lw-us.apache.org (ASF Mail Server at mailrelay1-lw-us.apache.org) with ESMTPSA id 6540C106F for ; Sat, 11 Jan 2020 01:23:45 +0000 (UTC) Received: by mail-ot1-f54.google.com with SMTP id 77so3798266oty.6 for ; Fri, 10 Jan 2020 17:23:45 -0800 (PST) X-Gm-Message-State: APjAAAWf644Gf2Jzcqyvg5RZwV0b1WQyP4GlfQ+qPaV6AAPIiK4Cj8Gn 4b0DzV/+Klug0OlsMM40VZa9n6wUzpcwpF9yZC8= X-Google-Smtp-Source: APXvYqyQnw1yVoxFj3zQ+25XlDiEm7TfC61sV5YZcyvkBWE6fckFu3hFIU+4QPDCTUnF5BV7aPGW8y9Jc3epibqDkhk= X-Received: by 2002:a05:6830:10a:: with SMTP id i10mr4881179otp.365.1578705824827; Fri, 10 Jan 2020 17:23:44 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Andrew Purtell Date: Fri, 10 Jan 2020 17:23:09 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [DISCUSS] Guidelines for Code cleanup JIRAs To: dev Content-Type: multipart/alternative; boundary="000000000000e4cd57059bd31776" --000000000000e4cd57059bd31776 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable ... 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. On Fri, Jan 10, 2020 at 5:15 PM Andrew Purtell 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 > 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 ta= kes >> 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. W= e >> have three active branches: master, branch-2, branch-1. The more they >> diverge, the more work for everyone for every commit. "Cleanups" for the= ir >> 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 wrote= : >> >>> On Fri, Jan 10, 2020 at 16:53 Andrew Purtell >>> 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 lo= w >>> > priority findbugs findings. >>> >>> >>> I=E2=80=99m 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 =E2= =80=94 >>> there=E2=80=99s >>> really no excuse for contributors ignoring the warnings they produce. I= f >>> we >>> don=E2=80=99t like some class of warning, we should adjust the rule, no= t ignore >>> the >>> check failure. >>> >>> I agree there=E2=80=99s 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=E2=80=99s >>> recent efforts to address the checkstyle issues, module by module. >>> >>> On Fri, Jan 10, 2020 at 4:45 PM Nick Dimiduk >>> wrote: >>> > >>> > > Thanks for being this up Andrew. >>> > > >>> > > I am also +1 for code cleanup. I agree that such efforts must hit t= he >>> > fork >>> > > branches of each release line, thus: master, branch-2, branch1. >>> > > >>> > > I=E2=80=99m -0 on taking such commits to release branches. These co= de 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 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 u= p >>> > 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 >> > >>> > > 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 multip= le >>> > 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 righ= t >>> 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 no= t >>> 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 land= s >>> 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 > --=20 Best regards, Andrew Words like orphans lost among the crosstalk, meaning torn from truth's decrepit hands - A23, Crosstalk --000000000000e4cd57059bd31776--