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: Re: Clean up the all the checkstyle violations in the zookeeper-server module
Date Tue, 16 Jul 2019 07:17:24 GMT
The main concern here is that we have already too
many issues on enable specific rules on zookeeper,
including ZOOKEEPER-3434 and ZOOKEEPER-3446,
and it would be quite noisy to enable per rule(as
been 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-3431
> has a specific description and I'm afraid it could not
> be an umbrella issue.
>
> How about close all checkstyle related issues and start
> a 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