zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Justin Ling Mao" <maoling199210...@sina.com>
Subject Re: Re: Re: Re: Re: Clean up the all the checkstyle violations in the zookeeper-server module
Date Tue, 16 Jul 2019 10:19:12 GMT
-1. @Zili Chen   I had linked ZOOKEEPER-3434(closed),ZOOKEEPER-3446 to the ZOOKEEPER-3431 which
now is a Umbrella JIRA (Type:Task).   I will also take your advice about the subtasks.-2 --->"Please
add jute and Prometheus module"   @Olivelli.That's OK.

----- Original Message -----
From: Zili Chen <wander4096@gmail.com>
To: DevZooKeeper <dev@zookeeper.apache.org>
Cc: maoling199210191@sina.com
Subject: Re: Re: Re: Re: Clean up the all the checkstyle violations in the zookeeper-server
module
Date: 2019-07-16 15:18

The main concern here is that we have already toomany issues on enable specific rules on zookeeper,including
ZOOKEEPER-3434 and ZOOKEEPER-3446,and it would be quite noisy to enable per rule(asbeen described
and reached a consensus).
Best,tison.

Zili Chen <wander4096@gmail.com> 于2019年7月16日周二 上午9:17写道:
Hi Justin,
Thanks for driving this thread. Please go ahead!
One thing I'd like to pick up is that ZOOKEEPER-3431has a specific description and I'm afraid
it could notbe an umbrella issue.
How about close all checkstyle related issues and starta new issues structure as
Umbrella: Enable Google checkstyle configuration  Subtask-1: Add silent Google checkstyle
configuration  Subtask-2: Enable Google checkstyle configuration on zookeeper-server  Subtask-3:
Enable Google checkstyle configuration on zookeeper-jute
  Subtask-4: Enable Google checkstyle configuration on zookeeper-prometheus
  ...
Best,tison.

Enrico Olivelli <eolivelli@gmail.com> 于2019年7月16日周二 上午12:06写道:
Il lun 15 lug 2019, 09:14 Justin Ling Mao <maoling199210191@sina.com> ha

scritto:



> - any advance for the discussion???- any objections about these two

> things: 1.only clean the main-module:zookeeper-server;





Please add jute and Prometheus module



2.using the google's checkstyle_style?-





Works for me



> who will head it up?  how about me?

>



Sure! Go for it. Thanks



Enrico



>

>

> ----- Original Message -----

> From: "Justin Ling Mao" <maoling199210191@sina.com>

> To: "dev" <dev@zookeeper.apache.org>

> Subject: Re: Re: Re: Clean up the all the checkstyle violations in the

> zookeeper-server module

> Date: 2019-07-07 15:56

>

> 1.--->“we'd better first create an umbrella issue named "Enable checkstyle

> rules" or sth”I had created ZOOKEEPER-3431 previously, and we can create a

> series of sub-tasks under it.

> 2.I think we still have two things which should be discussed:  2.1

> Currently, we only need to enforce the checkstyle violations check in the

> main-module:zookeeper-server, not included other modules?      IMO, because

> the zookeeper-contrib, zookeeper-recipes are now not well-maintained.

> and some violations in the zookeeper-jute are auto-generated. so focusing

> on zookeeper-server is enough?

>   2.2 What checkstyle template we will pick up? Now we have three

> options:      A:[google_style](

> https://checkstyle.sourceforge.io/google_style.html)

> B:[bookkeeper_style] (

> https://github.com/apache/bookkeeper/blob/master/buildtools/src/main/resources/bookkeeper/checkstyle.xml)

>     C:[hbase_style](

> https://github.com/apache/hbase/blob/master/hbase-checkstyle/src/main/resources/hbase/checkstyle.xml)

>     Which one will we choose?

>

>

> ----- Original Message -----

> From: Enrico Olivelli <eolivelli@gmail.com>

> To: dev@zookeeper.apache.org

> Cc: maoling199210191@sina.com

> Subject: Re: Re: Clean up the all the checkstyle violations in the

> zookeeper-server module

> Date: 2019-07-07 15:13

>

> Il dom 7 lug 2019, 01:29 Zili Chen <wander4096@gmail.com> ha scritto:

> > Justin & Enrico,

> >

> > Receiving no opposition on this proposal, we could regard it as

> > a consensus. According to bookkeeper#230 we'd better first create

> > an umbrella issue named "Enable checkstyle rules" or sth. Under

> > there we can finally decide the checkstyle configuration and

> > start sub-tasks enabling per package.

> >

> > For keeping current checkstyle, I'd like to pick up that it's

> > possible that we remain the current simple config for all pkgs,

> > adding a config said copied from bookkeeper named

> > "strict-checkstyle.xml", enabling per pkg, which contains @author

> > tags and rules in simple config. Once we enabling the strict one

> > for all pkgs. We can merge two configs into one.

> >

> +1 please go ahead

> Enrico

> > 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