Enrico, Sounds reasonable since checkstyle is part of building cycle. Thanks!
Best, tison. Enrico Olivelli <[email protected]> 于2019年7月17日周三 下午6:01写道： > Zili, > I think 'build' is a good option > > Enrico > > Il giorno mer 17 lug 2019 alle ore 11:18 Zili Chen <[email protected]> > ha scritto: > > > Andor & Enrico, > > > > I find there is no proper JIRA component this thread can be put under. > > > > Any advise?(Both "document" or "server" used now seems not quite > accurate. > > > > Best, > > tison. > > > > > > Zili Chen <[email protected]> 于2019年7月16日周二 下午6:28写道： > > > > > Hi Justin, > > > > > > Thanks for the reply. > > > > > > I will close ZOOKEEPER-3446 later in fact. Because > > > our consensus above is that we would add the checkstyle > > > configuration at first and enable it per module. > > > > > > I'm glad to follow ZOOKEEPER-3431 to see if anything I > > > can help with. > > > > > > Best, > > > tison. > > > > > > > > > Justin Ling Mao <[email protected]> 于2019年7月16日周二 下午6:19写道： > > > > > >> -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 <[email protected]> > > >> To: DevZooKeeper <[email protected]> > > >> Cc: [email protected] > > >> 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 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 <[email protected]> 于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 <[email protected]> 于2019年7月16日周二 上午12:06写道： > > >> > > >> Il lun 15 lug 2019, 09:14 Justin Ling Mao <[email protected]> > > 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" <[email protected]> > > >> > To: "dev" <[email protected]> > > >> > 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 <[email protected]> > > >> > To: [email protected] > > >> > Cc: [email protected] > > >> > 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 <[email protected]> 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 <[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. > > >> > > > > > > >>>>>> > > >> > > > > > > >>>>> > > >> > > > > > > >>>> > > >> > > > > > > >>> > > >> > > > > > > >> > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > >
