hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Purtell <apurt...@apache.org>
Subject Re: [DISCUSS] Guidelines for Code cleanup JIRAs
Date Sat, 11 Jan 2020 01:23:09 GMT
... 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 <apurtell@apache.org> 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 <apurtell@apache.org>
> 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 <ndimiduk@apache.org> wrote:
>>
>>> On Fri, Jan 10, 2020 at 16:53 Andrew Purtell <apurtell@apache.org>
>>> 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 <ndimiduk@apache.org>
>>> 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 <busbey@apache.org>
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 <apurtell@apache.org
>>> >
>>> > > 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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message