ZOOKEEPER-3464:enfore checkstyle in the zookeeper-server module and clean the package:admin and client

[maoling]

• this PR involves two package: admin and client and 7 java files. Use the IDEA git diff tool, we can easily check that I only clean the code format, not change any code logic.
• Go to download the Plugins: checkstyle-IDEA and google-java-format (which allows users to
  format codes with the keyboard shortcut: Ctrl + Alt + L),Otherwise, cleaning this things manually will make you crazy:)
• the following rules have been cut off from the google checkstyle for some reality:
  • AbbreviationAsWordInName .some classes or fields like ZKClientConfig cannot pass this rule and I cannot refactor the name for the compatibility issue
  • JavadocParagraph, the ASF licenses head cannot pass this rule
• more details in the ZOOKEEPER-3464

[maoling]

• mvn clean apache-rat:check install -DskipTests spotbugs:check checkstyle:check -Pfull-build
  Can we wipe out the checkstyle:check, let my PR pass the travis-ci?

[eolivelli]

awesome work.

we have to fix the Maven checkstyle plugin configuration, please take a look to my comments

[eolivelli]

this way you are disabling the minimal checkstyle configuration I added to prevent the presence of '@author' tags.
As your new configuration is applied only to some limited list of packages you are implicitly disabling that check on a part of the codebase.

Please add a new '' of the plugin with your new checkstyle.xml and checkstyleSuppressions.xml

Please test manually that if you add a '@author' javadoc tag anywhere in code the PR validation fails

[maoling]

@eolivelli

• Done.use the separate checkstyle-strict.xml for the zookeeper-server module.
• I check that our code base doesn't have the '@author' tag

[tisonkun]

It is reasonable to use a checkstyle-strict.xml covering the simple config in pkgs we want to enable more rules.

For "I check that our code base doesn't have the '@author' tag", I think you should also test if a '@author' tag exists, the build fails with correct reason.

[maoling]

• @tisonkun Thanks for the notice.
• I check the rule for the "the '@author' tag"

        <!-- Checks that disallowed strings are not used in comments.  -->
        <property name="format" value="(@author)" />
    </module>

and found it doesn't work.search around and don't find a valid way to enforce no author tag in the javaDoc.Cc @eolivelli

[tisonkun]

Hi @maoling , you can try out enable the former simple checkstyle configuration in parent pom.xml and enable strict checkstyle configuration in, for example, zookeeper-server pkg. Make sure the strict checkstyle configuration contains rules in the simple one including forbid '@author' tag. I would provide a patch you can make use of in this week if you stuck.

[maoling]

@tisonkun
Nothing can make me get stuck, except the beautiful girls;)

[tisonkun]

@maoling you can take a look at ZOOKEEPER-3465 and #1022 #1023 where I show you how to enable checkstyle configuration per pkg and why I stand "no need" for extra suppression configuration.

You can rebase the subtasks enable it on zookeeper-server(i.e., this thread) on #1022 . Feel free to share you opinion because I did change the checkstyle configuration choice.

[tisonkun]

Thanks for this awesome work @maoling!

I have several concerns about the implementations.

1. Here we do two things at once, i.e., add a configuration file and enable it on zookeeper-server. As discussed in mailing list we would at first add a configuration and then enable it per pkg so there is ZOOKEEPER-3465. At least I think we should separate these two topics into different commits.

2. Could you explain where rules in checkstyleSuppressions-strict.xml comes from? If we follow the process above, we could just enable it per package and there should be no suppression config needed.

[maoling]

@tisonkun

• add a configuration file and enable it on zookeeper-server. As discussed in mailing list we would at first add a configuration and then enable it per pkg so there is ZOOKEEPER-3465

• It's reasonable, but mixing them will not give us too much code review burden.The most important is:adding and enabling the configuration file without picking a package to do the experimental cleaning work will not give me the insight and check/confirm what the things I'am doing is correct.

• Could you explain where rules in checkstyleSuppressions-strict.xml comes from?

• every package under the zookeeper-server is a suppressions statement, but the 37 java files under that module in the first level is difficult to write the suppressions regex and their codes are so long and many, many violates, so I split them into the specify java files for the CR easily in the future.

• If we follow the process above, we could just enable it per package and there should be no suppression config needed.

• I only know the suppression way, how to enable it per package? plz tell me.

[eolivelli]

I think we can close this pr.

cc @tisonkun

[tisonkun]

Sure.

Follow the current progress whoever proceed the work on zookeeper-server would do it quite different from this pull request.