zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zili Chen <wander4...@gmail.com>
Subject Re: Re: Clean up the all the checkstyle violations in the zookeeper-server module
Date Thu, 18 Jul 2019 16:36:25 GMT
Hi Enrico,

I see your concern. My question is, how does "a new execution of checkstyle"
implement? My experience with checkstyle plugin is one pass to check all
rules so I'm curiously whether we add another profile to execute or
in other ways.

Best,
tison.


Enrico Olivelli <eolivelli@gmail.com> 于2019年7月18日周四 下午8:46写道:

> Hi Tison,
>
>
>
> Il gio 18 lug 2019, 10:40 Zili Chen <wander4096@gmail.com> ha scritto:
>
> > Hi Enrico,
> >
> > I just noticed that you have mentioned
> >
> > We can keep current checkstyle invokation that checks for @author tags
> as a
> > separate 'execution' of the plugin with a specific checkstyle file (as
> you
> > already said)
> >
> > I'm curious how "a separated execution" achieves? I found
> > that checkstyle plugin can be only configured one checkstyle
> > conifg files and thus propose the current simple config applied
> > on all modules and a strict config covering the simple one
> > applied per package(server, prometheus, and jute as discussed).
> >
>
>
> We already have one checkstyle file and it must be checked against the
> whole codebase, ad it looks for @author tags.
>
> If we add new rules to the existing file they will be enabled for all of
> the modules.
> If you limit checkstyle to only some package we will lose current checks
>
> So my idea is to add a new execution of checkstyle with a new checkstyle
> configuration file, for this new execution of the plugin we will have:
> - strict  checks
> - selective per-package abilitation of checkstyle
>
>
>
> >
> > Best,
> > tison.
> >
> >
> > Enrico Olivelli <eolivelli@gmail.com> 于2019年7月6日周六 下午8:20写道:
> >
> > > Justin,
> > > This is how we did it in Bookkeeper, we enabled checkstyle only for
> group
> > > of packages in the main module (the biggest one, bookkeeper-server)
> > >
> > > https://github.com/apache/bookkeeper/issues/230
> > >
> > > I suggest using that checkstyle config, I feel we won't have so many
> > > violations.
> > >
> > > We can keep current checkstyle invokation that checks for @author tags
> > as a
> > > separate 'execution' of the plugin with a specific checkstyle file (as
> > you
> > > already said)
> > >
> > > I am happy to help, thank you for driving this effort
> > >
> > > Enrico
> > >
> > >
> > > Il sab 6 lug 2019, 11:33 Justin Ling Mao <maoling199210191@sina.com>
> ha
> > > scritto:
> > >
> > > > - 1.It seems that we had reached a consensus to work on this.- 2.I
> also
> > > > agree on the way: fix one package at a time, then another.- 3.Now Let
> > us
> > > > discuss some details:    - 3.1 how to make the checkstyle only check
> > the
> > > > package we specify? I found this:      URL:
> > > >
> > >
> >
> https://stackoverflow.com/questions/26455174/only-enable-some-checks-for-certain-inner-package
> > > >     @Olivelli Could you give me more your insight?    - 3.2 What
> rules
> > > will
> > > > we init in the checkstyle.xml?       3.2.1 - I also think the rules
> > from
> > > > the hbase is too strict which will cause too many,many violations.
> > > >  3.2.2 - apply the "Google's Java Style Checkstyle Coverage" is a
> good
> > > > option? which seems to be more simplify and more suitable for us?
> > > >      URL:https://checkstyle.sourceforge.io/google_style.html
> > > >
> > > >
> > > >
> > > > ----- Original Message -----
> > > > From: Andor Molnar <andor@cloudera.com.INVALID>
> > > > To: DevZooKeeper <dev@zookeeper.apache.org>
> > > > Subject: Re: Clean up the all the checkstyle violations in the
> > > > zookeeper-server module
> > > > Date: 2019-07-02 13:22
> > > >
> > > > Yes. That way we only need to fix one package at a time.
> > > > Andor
> > > > On Mon, Jul 1, 2019 at 4:10 PM Zili Chen <wander4096@gmail.com>
> wrote:
> > > > > Hi Andor,
> > > > >
> > > > > To be exact, "iterations" means we define the original rules
> > > > > in checkstyle configuration at once and turn them on one package
> > > > > after another, so iterations. Is it correct?
> > > > >
> > > > > Best,
> > > > > tison.
> > > > >
> > > > >
> > > > > Andor Molnar <andor@apache.org> 于2019年7月1日周一 下午9:09写道:
> > > > >
> > > > > > I like the idea of doing this in iterations.
> > > > > >
> > > > > > Andor
> > > > > >
> > > > > >
> > > > > >
> > > > > > > On 2019. Jun 29., at 8:35, Zili Chen <wander4096@gmail.com>
> > wrote:
> > > > > > >
> > > > > > > A solution could be, we remains current simple configuration
> > > > > > > and introduce a so-called "strict-checkstyle.xml" and apply
> > > > > > > it per package. Once we enable it on every package, we
can
> > > > > > > merge it into the simple configuration.
> > > > > > >
> > > > > > > Best,
> > > > > > > tison.
> > > > > > >
> > > > > > >
> > > > > > > Enrico Olivelli <eolivelli@gmail.com> 于2019年6月29日周六
下午2:19写道:
> > > > > > >
> > > > > > >> Il sab 29 giu 2019, 07:59 Zili Chen <wander4096@gmail.com>
ha
> > > > > scritto:
> > > > > > >>
> > > > > > >>> Thank you Enrico. It seems my previous reply delivered
> > > > > > >>> into another thread. Repost below.
> > > > > > >>>
> > > > > > >>> Hi zookeepers,
> > > > > > >>>
> > > > > > >>> I have proceeded ZOOKEEPER-3446 and been guided
to here
> > > > > > >>> to discuss for a consensus before any more efforts.
> > > > > > >>>
> > > > > > >>> In general, +1 on introducing and forcing checkstyle.
> > > > > > >>>
> > > > > > >>> About the process, I agree that we firstly reach
a consensus
> > > > > > >>> on the configuration and enable it per package.
In order for
> > > > > > >>> our contributors to rebase as few times as possible,
we'd
> > > > > > >>> better introduce all rules we all agree on at once.
Note that
> > > > > > >>> we could always add rule if someone ask for and
agreed by the
> > > > > > >>> community.
> > > > > > >>>
> > > > > > >>> Currently, we turn on checkstyle on all modules.
> > > > > > >>
> > > > > > >>
> > > > > > >> This is quite important because we are using inlt in
order to
> > > > prevent
> > > > > > >> @author tags.
> > > > > > >> This was part of the ant based precommit job
> > > > > > >>
> > > > > > >> Enrico
> > > > > > >>
> > > > > > >>
> > > > > > >>
> > > > > > >>
> > > > > > >> Following the
> > > > > > >>> process above, we firstly turn off it once we apply
the new
> > > > > > >>> configuration, and then turn on it per package.
> > > > > > >>>
> > > > > > >>> If the community is willing to do this work, a
JIRA about the
> > new
> > > > > > >>> checkstyle configuration should be filed and we
continue the
> > > > > discussion
> > > > > > >>> there. Generally, rules proposed by Enrico are
good start
> point
> > > and
> > > > > > >>> I agree on we should not introduce anything "fancy",
but
> > > according
> > > > to
> > > > > > >>> what is actually needed.
> > > > > > >>>
> > > > > > >>> Best,
> > > > > > >>> tison.
> > > > > > >>>
> > > > > > >>>
> > > > > > >>> Enrico Olivelli <eolivelli@gmail.com> 于2019年6月29日周六
> 下午1:51写道:
> > > > > > >>>
> > > > > > >>>> @Zili Chen
> > > > > > >>>> This is the original email thread. So you can
answer here.
> > > > > > >>>>
> > > > > > >>>> Enrico
> > > > > > >>>>
> > > > > > >>>> Il ven 28 giu 2019, 11:04 Norbert Kalmar
> > > > > <nkalmar@cloudera.com.invalid
> > > > > > >>>
> > > > > > >>> ha
> > > > > > >>>> scritto:
> > > > > > >>>>
> > > > > > >>>>> Community is eager to jump on on this,
we already have pull
> > > > > requests
> > > > > > >>>>> cleaning up imports :)
> > > > > > >>>>>
> > > > > > >>>>> First of all, sorry for the late reply
(I thought I already
> > > > > answered
> > > > > > >>>> this,
> > > > > > >>>>> I remember reading it and drawing up an
answer. Oh well)
> > > > > > >>>>>
> > > > > > >>>>> Some big patches are already reviewed,
I think we should
> > commit
> > > > as
> > > > > > >> much
> > > > > > >>>> as
> > > > > > >>>>> possible before doing this refactor. (I'll
also try to rev
> up
> > > my
> > > > > code
> > > > > > >>>>> review/commit thread)
> > > > > > >>>>>
> > > > > > >>>>> As for waiting for 3.6.0 - I don't see
the reason we
> should.
> > > > Unless
> > > > > > >> of
> > > > > > >>>>> course this would delay the release too
much...
> > > > > > >>>>>
> > > > > > >>>>> I haven't checked HBase checkstyle against
our code, I
> don't
> > > > think
> > > > > we
> > > > > > >>>>> should introduce anything "fancy". What
Enrico listed up
> > sounds
> > > > > like
> > > > > > >> a
> > > > > > >>>> good
> > > > > > >>>>> starting point.
> > > > > > >>>>>
> > > > > > >>>>> +1 on introducing and forcing checkstyle.
> > > > > > >>>>>
> > > > > > >>>>> Regards,
> > > > > > >>>>> Norbert
> > > > > > >>>>>
> > > > > > >>>>>
> > > > > > >>>>> On Sun, Jun 23, 2019 at 7:27 PM Enrico
Olivelli <
> > > > > eolivelli@gmail.com
> > > > > > >>>
> > > > > > >>>>> wrote:
> > > > > > >>>>>
> > > > > > >>>>>> Justin,
> > > > > > >>>>>> Thank you so much for your help in
this.
> > > > > > >>>>>>
> > > > > > >>>>>> I would suggest to apply all of the
rules in one pass,
> > > splitting
> > > > > > >> the
> > > > > > >>>> work
> > > > > > >>>>>> per package.
> > > > > > >>>>>> This way reviews will be easier, we
will limit the number
> of
> > > > > > >> commits
> > > > > > >>>> and
> > > > > > >>>>> we
> > > > > > >>>>>> won't annoy too much the contributors
, asking for hard
> > > rebases
> > > > > > >>>>>>
> > > > > > >>>>>> This is how we did it on Apache Bookkeeper
> > > > > > >>>>>> https://github.com/apache/bookkeeper/issues/230
> > > > > > >>>>>>
> > > > > > >>>>>> I will help review and commit all of
your patches,  it
> will
> > be
> > > > > > >>> mostly a
> > > > > > >>>>>> matter of code reformat without any
behavior change.
> > > > > > >>>>>> Currently I am doing the same kind
of work on others
> > projects
> > > of
> > > > > my
> > > > > > >>>>>> company, so I perfectly know how the
work will go.
> > > > > > >>>>>>
> > > > > > >>>>>> Before starting we must ensure that:
> > > > > > >>>>>> 1) community is willing to do this
work (we will force a
> > > rebase
> > > > on
> > > > > > >>>> mostly
> > > > > > >>>>>> every pending PR)
> > > > > > >>>>>> 2) the proposed configuration is accepted
by the community
> > > > > > >>>>>> 3) it is the good time to do it, or
should we wait for
> 3.6.0
> > > to
> > > > be
> > > > > > >>> out
> > > > > > >>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>> I see you are referring to hbase checkstyle
file,  did
> you
> > > > > already
> > > > > > >>>>> checked
> > > > > > >>>>>> how much different it is from current
project style?
> > > > > > >>>>>> Will we only need to remove trailing
spaces, reorder
> > members,
> > > > fix
> > > > > > >>>>> imports,
> > > > > > >>>>>> cut long lines ?
> > > > > > >>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>> Cheers
> > > > > > >>>>>> Enrico
> > > > > > >>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>> Il dom 23 giu 2019, 15:11 Justin Ling
Mao <
> > > > > > >> maoling199210191@sina.com
> > > > > > >>>>
> > > > > > >>>> ha
> > > > > > >>>>>> scritto:
> > > > > > >>>>>>
> > > > > > >>>>>>> Background:zookeeper-server is
the main-module of the zk
> > > > > > >> codebase.
> > > > > > >>>>>>> Unfortunately, there were many
checkstyle violations in
> it.
> > > To
> > > > > > >>>> improve
> > > > > > >>>>>> the
> > > > > > >>>>>>> code quality and code standards,
> > > > > > >>>>>>> IMHO, it's time to clean up the
all the checkstyle
> > > > > > >> violations(turn
> > > > > > >>> on
> > > > > > >>>>> the
> > > > > > >>>>>>> <failOnViolation>true</failOnViolation>).
we can learn
> from
> > > the
> > > > > > >>> hbase
> > > > > > >>>>>> whose
> > > > > > >>>>>>> checkstyle(almost 40+ rules) is
very strict and ensures a
> > > very
> > > > > > >>>> unified
> > > > > > >>>>>> code
> > > > > > >>>>>>> style.(
> > > > > > >>>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>
> > > > > > >>>>
> > > > > > >>>
> > > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/hbase/blob/master/hbase-checkstyle/src/main/resources/hbase/checkstyle.xml
> > > > > > >>>>>>> )
> > > > > > >>>>>>> My planing is: clean up the all
the checkstyle one rule
> by
> > > > > > >> another
> > > > > > >>> to
> > > > > > >>>>>>> avoid too much code changes for
review.
> > > > > > >>>>>>> Everything's hard in the beginning,
I have fired my first
> > > shot(
> > > > > > >>>>>>> https://github.com/apache/zookeeper/pull/992).
> > > > > > >>>>>>> If this draft has accepted by the
community, I will
> create
> > > the
> > > > > > >>>>>>> corresponding sub-tasks for more
people joining this
> work.
> > > > > > >>>>>>>
> > > > > > >>>>>>> Cited the comment from Enrico Olivelli
in the
> > > > > > >>>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>
> > > > > > >>>>
> > > > > > >>>
> > > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
> ZOOKEEPER-3431:--------------------------------------------------------------we
> > > > > > >>>>>>> have to discuss this topic with
the community.I have
> > > experience
> > > > > > >> on
> > > > > > >>>>>>> BookKeeper, we had to clean up
groups of packages.This is
> > the
> > > > > > >> kind
> > > > > > >>> of
> > > > > > >>>>>> stuff
> > > > > > >>>>>>> that invalidates all of the pending
pull requests.I have
> > > > already
> > > > > > >>> sett
> > > > > > >>>>> up
> > > > > > >>>>>> a
> > > > > > >>>>>>> basic checkstyle configuration
file and it is already
> > active
> > > > but
> > > > > > >> it
> > > > > > >>>> is
> > > > > > >>>>>>> performing only very basic checks
(like no 'author'
> tags).I
> > > > will
> > > > > > >>>>>> appreciate
> > > > > > >>>>>>> very much if you want to drive
this effort, personally I
> > > would
> > > > > > >>> start
> > > > > > >>>>> this
> > > > > > >>>>>>> stuff after 3.6.0 release, once
we consolidate current
> > master
> > > > and
> > > > > > >>> the
> > > > > > >>>>>> maven
> > > > > > >>>>>>> build. I would have sent an email
on the dev@ list
> soon.We
> > > > also
> > > > > > >>> have
> > > > > > >>>>> to
> > > > > > >>>>>>> agree on the checkstyle configuration,
it is not
> trivial, I
> > > > would
> > > > > > >>>> take
> > > > > > >>>>>> the
> > > > > > >>>>>>> file from HBase, BookKeeper or
other ASF projects on the
> > > > Haddoop
> > > > > > >>>>>> ecosystem.
> > > > > > >>>>>>
> > > > > > >>>>>
> > > > > > >>>>
> > > > > > >>>
> > > > > > >>
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

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