From dev-return-81876-archive-asf-public=cust-asf.ponee.io@zookeeper.apache.org Sun Jul 7 06:46:20 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 94B8318063F for ; Sun, 7 Jul 2019 08:46:19 +0200 (CEST) Received: (qmail 41392 invoked by uid 500); 7 Jul 2019 06:46:18 -0000 Mailing-List: contact dev-help@zookeeper.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zookeeper.apache.org Delivered-To: mailing list dev@zookeeper.apache.org Received: (qmail 41380 invoked by uid 99); 7 Jul 2019 06:46:18 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd2-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 07 Jul 2019 06:46:18 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd2-us-west.apache.org (ASF Mail Server at spamd2-us-west.apache.org) with ESMTP id 8828F1A32D1 for ; Sun, 7 Jul 2019 06:46:17 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd2-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.801 X-Spam-Level: * X-Spam-Status: No, score=1.801 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=2, PDS_NO_HELO_DNS=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=disabled Authentication-Results: spamd2-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-ec2-va.apache.org ([10.40.0.8]) by localhost (spamd2-us-west.apache.org [10.40.0.9]) (amavisd-new, port 10024) with ESMTP id TdOQjeffU37q for ; Sun, 7 Jul 2019 06:46:15 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=209.85.208.179; helo=mail-lj1-f179.google.com; envelope-from=eolivelli@gmail.com; receiver= Received: from mail-lj1-f179.google.com (mail-lj1-f179.google.com [209.85.208.179]) by mx1-ec2-va.apache.org (ASF Mail Server at mx1-ec2-va.apache.org) with ESMTPS id 821C1BC52B for ; Sun, 7 Jul 2019 06:46:14 +0000 (UTC) Received: by mail-lj1-f179.google.com with SMTP id r9so12828104ljg.5 for ; Sat, 06 Jul 2019 23:46:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=PvzRGJW1jSfzSZbaInk6y9NOZeBu7y13k8KHcDsIzSo=; b=I0wIZxdqSNSfs/58j9W3tf7vOKpLWdwVq0UUFBkHOop4CvdxLCWYqluh2fOenRBS2f vswKz5pES5fYE1lf8g32CN8bmofneOPtx6iIDxbt2PLIbncEiOoTVcs1fGv9RyV+am5Z TnZdyQdSQoZJvPZs+9pMN59QeLQ/axlZQz9LBt8zAouNkrftBUyT9X7V1xosMYwF3mJN 83G5CWQ8ecV7b8nl5I8BKCg2EwIc0ZQlhNSCojAsVeBihx7gF1l1xWqIqKFt2HxviTam hbaqCdkIdRzVMZADsms1+HCKucnlmBazfz69JAB80zFwrlHcJEgiVUf6FM/CLO/Ygeqn r6yw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=PvzRGJW1jSfzSZbaInk6y9NOZeBu7y13k8KHcDsIzSo=; b=YW143vR/H+xmvZ3Utw3eEZkbR2IEYl8LiYJQrjG72x7Up1slPbDSaFpdENX6PEuvPi 586qBMZny6pibLRxbDPHI5NfZw7P1RThhDmssCfo7HJzS0VWvzki7AralD/xu5StXZho VE+a0c0hP8lC7w2pPJ7r0C6nVqFe31YEgheQa2SgB3yx1u4RNwMpbjMG9kcXmNTqUE4v 1lM0lQfedid7n4eO7Iv/mJ8MjC+MpUq5coOsJvWgmmE5Qmq83aEPiE/J/md8rtC4NrJj 71y6eQq+SKT3EVhTHDVdpPGRpeECuUPLe459W23GCt75li+cwGh86/hjQXlYDlcOSuZ0 p/Ug== X-Gm-Message-State: APjAAAVBRcxKL7tdB6q40VcSw4PzMx/QvnwLyBGzLZfh4Y2VNXi7vnb2 +058HTViqJKrySqZfXCyVSWnU+GMgJ71qrdg3i9vGg== X-Google-Smtp-Source: APXvYqxRatl063TSi4pDLFQBTZ4VblP6/6H2koC8bOqwZac3zqplIuvbAYlC6Wl6ftfCYD5eMDtrks2eT8GxX1y4dK4= X-Received: by 2002:a2e:8744:: with SMTP id q4mr6497290ljj.77.1562481966202; Sat, 06 Jul 2019 23:46:06 -0700 (PDT) MIME-Version: 1.0 References: <20190706093259.EFCE04D2@webmail.sinamail.sina.com.cn> In-Reply-To: From: Enrico Olivelli Date: Sun, 7 Jul 2019 08:45:54 +0200 Message-ID: Subject: Re: Re: Clean up the all the checkstyle violations in the zookeeper-server module To: dev@zookeeper.apache.org Cc: maoling199210191@sina.com Content-Type: multipart/alternative; boundary="0000000000009044d8058d11ae47" --0000000000009044d8058d11ae47 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Il dom 7 lug 2019, 01:29 Zili Chen 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 =E4=BA=8E2019=E5=B9=B47=E6=9C=886= =E6=97=A5=E5=91=A8=E5=85=AD =E4=B8=8B=E5=8D=888:20=E5=86=99=E9=81=93=EF=BC= =9A > > > Justin, > > This is how we did it in Bookkeeper, we enabled checkstyle only for gro= up > > 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 ha > > scritto: > > > > > - 1.It seems that we had reached a consensus to work on this.- 2.I al= so > > > 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 rule= s > > 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 goo= d > > > 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 > > > To: DevZooKeeper > > > 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 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 =E4=BA=8E2019=E5=B9=B47=E6=9C=881= =E6=97=A5=E5=91=A8=E4=B8=80 =E4=B8=8B=E5=8D=889:09=E5=86=99=E9=81=93=EF=BC= =9A > > > > > > > > > I like the idea of doing this in iterations. > > > > > > > > > > Andor > > > > > > > > > > > > > > > > > > > > > On 2019. Jun 29., at 8:35, Zili Chen > 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 =E4=BA=8E2019=E5=B9=B46= =E6=9C=8829=E6=97=A5=E5=91=A8=E5=85=AD =E4=B8=8B=E5=8D=882:19=E5=86=99=E9= =81=93=EF=BC=9A > > > > > > > > > > > >> Il sab 29 giu 2019, 07:59 Zili Chen 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=EF=BC=8C +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 poi= nt > > and > > > > > >>> I agree on we should not introduce anything "fancy", but > > according > > > to > > > > > >>> what is actually needed. > > > > > >>> > > > > > >>> Best, > > > > > >>> tison. > > > > > >>> > > > > > >>> > > > > > >>> Enrico Olivelli =E4=BA=8E2019=E5=B9=B46= =E6=9C=8829=E6=97=A5=E5=91=A8=E5=85=AD =E4=B8=8B=E5=8D=881:51=E5=86=99=E9= =81=93=EF=BC=9A > > > > > >>> > > > > > >>>> @Zili Chen > > > > > >>>> This is the original email thread. So you can answer here. > > > > > >>>> > > > > > >>>> Enrico > > > > > >>>> > > > > > >>>> Il ven 28 giu 2019, 11:04 Norbert Kalmar > > > > > > > > >>> > > > > > >>> 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 wil= l > 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 yo= u > > > > 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 i= t. > > To > > > > > >>>> improve > > > > > >>>>>> the > > > > > >>>>>>> code quality and code standards, > > > > > >>>>>>> IMHO, it's time to clean up the all the checkstyle > > > > > >> violations(turn > > > > > >>> on > > > > > >>>>> the > > > > > >>>>>>> true). we can learn fr= om > > 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/res= ources/hbase/checkstyle.xml > > > > > >>>>>>> ) > > > > > >>>>>>> My planing is: clean up the all the checkstyle one rule b= y > > > > > >> 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 creat= e > > 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.W= e > > > 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. > > > > > >>>>>> > > > > > >>>>> > > > > > >>>> > > > > > >>> > > > > > >> > > > > > > > > > > > > > > > > > > > > --0000000000009044d8058d11ae47--