From dev-return-81823-archive-asf-public=cust-asf.ponee.io@zookeeper.apache.org Tue Jul 2 05:22:22 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 5BA5A180670 for ; Tue, 2 Jul 2019 07:22:22 +0200 (CEST) Received: (qmail 64213 invoked by uid 500); 2 Jul 2019 05:22:21 -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 64199 invoked by uid 99); 2 Jul 2019 05:22:19 -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; Tue, 02 Jul 2019 05:22:19 +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 343CE1A323B for ; Tue, 2 Jul 2019 05:22:19 +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, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=disabled Authentication-Results: spamd2-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=cloudera.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 Uu48j-ZBUF_a for ; Tue, 2 Jul 2019 05:22:17 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=209.85.221.43; helo=mail-wr1-f43.google.com; envelope-from=andor@cloudera.com; receiver= Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.43]) by mx1-ec2-va.apache.org (ASF Mail Server at mx1-ec2-va.apache.org) with ESMTPS id D6F1FBC782 for ; Tue, 2 Jul 2019 05:22:16 +0000 (UTC) Received: by mail-wr1-f43.google.com with SMTP id c2so16159307wrm.8 for ; Mon, 01 Jul 2019 22:22:16 -0700 (PDT) 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; bh=JosO1+qSM/TWy/4qlMg/ovYuYZ5Z+Erw8PavG6Lm8xo=; b=S+dSBtofP/hP8oh4fYc1I6s3q+fLamg8Lskq9Pig+8GGo8wt2wt7rYqFUn+gi3WMQy DisIZiYlYZjkdHr0G8U98SyJYczlzoWsl7h03ztchmZY+l+0QepwbeEF5HpiHScTjP2H ObWVm2YZpCRsZBktqScodUN7BrpYJGeWkPqX7lCCywG0vLInDI++Tq0HU6lYyugsP6kH PhXIIdGli6kPt5qmsDb9N3uXdhAfA8LLCKeDXihiEw2Ee1GQXeKXkBfaGxTNqlCz5tPd +5GRulQRbGh325vw/RwJd8BALS/50eGS6pySHjMOyDZMvWDoGBgz0k25hQSrUwaXzBIs CtMA== X-Gm-Message-State: APjAAAWT1ua/Gc4SzBkMG/MVaI4XS++BhKRzypwv1QQ3QvyUxaiL625v IhtO7qhx8N3MuVhk50Wvpr5/j+NMaXy2+qUSi3HjE2XT/iw= X-Google-Smtp-Source: APXvYqxuUlI4ClKDt1lNeXf6N+Aytt0D3IViiJJB5t7cZRIL+J3U68y0GmG7xZWwmfF3GLRgdkI6LM7923EiVDJW/x8= X-Received: by 2002:a5d:55c2:: with SMTP id i2mr1659359wrw.113.1562044935529; Mon, 01 Jul 2019 22:22:15 -0700 (PDT) MIME-Version: 1.0 References: <20190623131048.33E1F19000E1@webmail.sinamail.sina.com.cn> In-Reply-To: From: Andor Molnar Date: Tue, 2 Jul 2019 07:22:04 +0200 Message-ID: Subject: Re: Clean up the all the checkstyle violations in the zookeeper-server module To: DevZooKeeper Content-Type: multipart/alternative; boundary="000000000000818e9d058cabed2a" --000000000000818e9d058cabed2a Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 preven= t > > >> @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 =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 a= s > > >> 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. Unle= ss > > >> of > > >>>>> course this would delay the release too much... > > >>>>> > > >>>>> I haven't checked HBase checkstyle against our code, I don't thin= k > 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, fi= x > > >>>>> 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 > > >>>>>>> true). 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/res= ources/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 alread= y > > >>> sett > > >>>>> up > > >>>>>> a > > >>>>>>> basic checkstyle configuration file and it is already active bu= t > > >> it > > >>>> is > > >>>>>>> performing only very basic checks (like no 'author' tags).I wil= l > > >>>>>> 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 a= nd > > >>> 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 wou= ld > > >>>> take > > >>>>>> the > > >>>>>>> file from HBase, BookKeeper or other ASF projects on the Haddoo= p > > >>>>>> ecosystem. > > >>>>>> > > >>>>> > > >>>> > > >>> > > >> > > > > > --000000000000818e9d058cabed2a--