hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Hentschel <jan.hentsc...@ultratendency.com>
Subject Re: [DISCUSS] Guidelines for Code cleanup JIRAs
Date Tue, 14 Jan 2020 20:59:09 GMT
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 <apurtell@apache.org>
Reply-To: "dev@hbase.apache.org" <dev@hbase.apache.org>
Date: Saturday, January 11, 2020 at 2:23 AM
To: dev <dev@hbase.apache.org>
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 <apurtell@apache.org<mailto: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<mailto: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<mailto:ndimiduk@apache.org>>
wrote:

On Fri, Jan 10, 2020 at 16:53 Andrew Purtell <apurtell@apache.org<mailto: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<mailto: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<mailto: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<mailto: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