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: Re: Clean up the all the checkstyle violations in the zookeeper-server module
Date Fri, 19 Jul 2019 07:32:13 GMT
lol

looking forward to your patch.

Best,
tison.


Justin Ling Mao <maoling199210191@sina.com> 于2019年7月19日周五 下午3:18写道:

> @Zili  don't worry about anything.I'am here.My first-shot patch will be
> coming at this weekend, then you can help me review it.
> ----- Original Message -----
> From: Zili Chen <wander4096@gmail.com>
> To: DevZooKeeper <dev@zookeeper.apache.org>
> Subject: Re: Re: Clean up the all the checkstyle violations in the
> zookeeper-server module
> Date: 2019-07-19 06:04
>
> 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