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