UP ! Hello ZooKeeper, Zili sent a PR to enable checkstyle on zookeeper-jute (1) The patch uses BookKeeper checkstyle file, as that style is similar to current ZooKeeper code style.
I need another at least another reviewer to validate the patch, this way we will unblock Zili and Justin work It would be better to make checkstyle land before cutting 3.6.0 release, or at least if we start (committing the first patch )then it is better to finish the work Enrico [1] https://github.com/apache/zookeeper/pull/1023 Il giorno ven 19 lug 2019 alle ore 09:32 Zili Chen <[email protected]> ha scritto: > lol > > looking forward to your patch. > > Best, > tison. > > > Justin Ling Mao <[email protected]> 于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 <[email protected]> > > To: DevZooKeeper <[email protected]> > > 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 <[email protected]> 于2019年7月18日周四 下午8:46写道： > > > Hi Tison, > > > > > > > > > > > > Il gio 18 lug 2019, 10:40 Zili Chen <[email protected]> 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 <[email protected]> 于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 < > [email protected]> > > > 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 <[email protected]> > > > > > > To: DevZooKeeper <[email protected]> > > > > > > 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 <[email protected]> > > > 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 <[email protected]> 于2019年7月1日周一 下午9:09写道： > > > > > > > > > > > > > > > I like the idea of doing this in iterations. > > > > > > > > > > > > > > > > Andor > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 2019. Jun 29., at 8:35, Zili Chen <[email protected] > > > > > > 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 <[email protected]> 于2019年6月29日周六 > > 下午2:19写道： > > > > > > > > > > > > > > > > > >> Il sab 29 giu 2019, 07:59 Zili Chen <[email protected] > > > > 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 <[email protected]> 于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 > > > > > > > <[email protected] > > > > > > > > >>> > > > > > > > > >>> 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 < > > > > > > > [email protected] > > > > > > > > >>> > > > > > > > > >>>>> 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 < > > > > > > > > >> [email protected] > > > > > > > > >>>> > > > > > > > > >>>> 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. > > > > > > > > >>>>>> > > > > > > > > >>>>> > > > > > > > > >>>> > > > > > > > > >>> > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
