Add spotless maven plugin to enforce java format. #6782

mqliang · 2021-04-13T02:58:58Z

Description

This change add a spotless maven plugin to enforce java format.

If java file is not formatted properly, all pipelines of Pinot Tests / Pinot Quickstart on JDK XXX will failed and hint developer to run mvn spotless:apply to fix the violation.

NOTE:
Currently, just add two basic rules:

Default import order
Remove unused imports

It's possible to add some more strict rules (via the goolge-formatter or the prettier-formatter ). For example, if we add the prettier formatter as follows in the plugin configuration:

       <configuration>
          <java>
            <includes>
              <include>src/main/java/**/*.java</include>
              <include>src/test/java/**/*.java</include>
            </includes>
            <importOrder />
            <removeUnusedImports />

            <prettier>
	      <devDependencies>
	          <prettier>2.0.5</prettier>
	          <prettier-plugin-java>0.8.0</prettier-plugin-java>
	      </devDependencies>
	      <config>
	          <tabWidth>2</tabWidth>
	          <parser>java</parser>
	      </config>
	    </prettier>

          </java>
        </configuration>

It will use 2 spaces as indention and format all function definition/calling code as using one line for each parameter, e.g.

public class HelloWorld {

  public static void main(String[] args) {
    System.out.println("Hello World!");
  }

  @Override
  public String toString() {
    sum(
    1, 
    2, 
    3, 
    4);
    return "Hello World";
  }

  public int sum(
    int argument1,
    int argument2,
    int argument3,
    int argument4,
    int argument5
  ) {
    return argument1 + argument2 + argument3 + argument4 + argument5;
  }
}

the goolge-formatter can not be used in our case, since it will mess up license headers, see google/google-java-format#168

@mcvsubbu @Jackie-Jiang @fx19880617 @siddharthteotia @mayankshriv I think we may need to discuss whether to adopt the code style of "one line for each parameter in function definition/calling code". I will add the prettier formatter configuration if need be.

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed

Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Documentation

mqliang · 2021-04-13T03:52:03Z

@mcvsubbu @Jackie-Jiang @fx19880617 @siddharthteotia @mayankshriv

Update: just found that the maven plugin support eclipse code formatter profile. Add the corresponding config and eclipse code style xml file in the 3rd commit. The xml file (config/eclipse-java-formatter.xml) is copied from the Apache avro project: https://github.com/apache/avro/blob/master/lang/java/eclipse-java-formatter.xml

You may noticed that we already have a config/codestyle-eclipse.xml file, but we can not use it directly, if we change the eclipse reformatter config as <file>${pinot.root}/config/codestyle-eclipse.xml</file>, mvn spotless:apply will failed with exception:

org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.9.0:apply (default-cli) on project pinot-spi: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:2.9.0:apply failed: java.lang.reflect.InvocationTargetException
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:215)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:156)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:148)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:117)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:81)
    at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build (SingleThreadedBuilder.java:56)
    at org.apache.maven.lifecycle.internal.LifecycleStarter.execute (LifecycleStarter.java:128)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:305)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:192)
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:105)
    at org.apache.maven.cli.MavenCli.execute (MavenCli.java:957)
    at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:289)
    at org.apache.maven.cli.MavenCli.main (MavenCli.java:193)
    at sun.reflect.NativeMethodAccessorImpl.invoke0 (Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke (NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke (Method.java:498)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:282)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:225)
    at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:406)
    at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:347)

I guess it's because the ${pinot.root}/config/codestyle-eclipse.xml is too stale (version=12, while version=15 for config/eclipse-java-formatter.xml copied from the Apache avro project.

In conclusion, there are 3 options to apply more strict formatting:

the goolge-formatter: can not be used in our case, since it will mess up license headers, see EPL license formatting google/google-java-format#168
the prettier-formatter: will format all function definition/calling code as each parameter use one line, as I described in Add spotless maven plugin to enforce java format. #6782 (comment).
eclipse code formatter: need add a new eclipse code formatter profile xml file (copied from Apache avro project), or update the current one we have, the current one we have is stale.

I prefer the 3rd option and update the the current codestyle-eclipse.xml as new content. Any thoughts?

mqliang · 2021-04-13T05:48:12Z

Test failed because LongMsgOffset.java placed license header incorrectly, send a PR #6783 to address it. Will rebase after #6783 is merged

codecov-io · 2021-04-13T06:27:57Z

Codecov Report

Merging #6782 (cb1ac89) into master (3f93cfb) will decrease coverage by 30.59%.
The diff coverage is 61.52%.

@@              Coverage Diff              @@
##             master    #6782       +/-   ##
=============================================
- Coverage     74.14%   43.55%   -30.60%     
+ Complexity       12        7        -5     
=============================================
  Files          1412     1412               
  Lines         68762    67930      -832     
  Branches       9934     9928        -6     
=============================================
- Hits          50986    29584    -21402     
- Misses        14461    35821    +21360     
+ Partials       3315     2525      -790

Flag	Coverage Δ	Complexity Δ
integration	`43.55% <61.52%> (-0.34%)`	`0.00 <0.00> (ø)`
unittests	`?`	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ava/org/apache/pinot/broker/api/AccessControl.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...apache/pinot/broker/api/HttpRequesterIdentity.java	`85.71% <ø> (ø)`	`0.00 <0.00> (ø)`
...org/apache/pinot/broker/api/RequestStatistics.java	`65.15% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...t/broker/api/resources/PinotBrokerHealthCheck.java	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...pinot/broker/api/resources/PinotBrokerRouting.java	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...ache/pinot/broker/broker/AccessControlFactory.java	`83.33% <ø> (ø)`	`0.00 <0.00> (ø)`
...ot/broker/broker/AllowAllAccessControlFactory.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
.../BrokerResourceOnlineOfflineStateModelFactory.java	`55.81% <ø> (ø)`	`0.00 <0.00> (ø)`
...not/broker/broker/helix/ClusterChangeMediator.java	`74.72% <ø> (-2.20%)`	`0.00 <0.00> (ø)`
...pinot/broker/pruner/PartitionZKMetadataPruner.java	`0.00% <0.00%> (-33.34%)`	`0.00 <0.00> (ø)`
... and 1496 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f93cfb...cb1ac89. Read the comment docs.

Jackie-Jiang · 2021-04-13T23:23:14Z

Nice work!
I would suggest first only add the rules for the import in the first PR.

We need to be very careful on the code style and keep it compatible with the current Pinot style. I believe the current eclipse style is compatible with the intellij style. We probably should update the existing eclipse style to the latest version. The end goal here should be giving the same result as the IDE auto-format.

mqliang · 2021-04-13T23:44:18Z

We need to be very careful on the code style and keep it compatible with the current Pinot style. I believe the current eclipse style is compatible with the intellij style. We probably should update the existing eclipse style to the latest version. The end goal here should be giving the same result as the IDE auto-format.

The codestyle-intellj.xml file can be deleted now. Intellj support import eclipse code formatter settings since 2014. I have tried
to import codestyle-eclipse.xml and it works well (maintaining one file is much easier that maintaining two). But I agree to add basic formatting rules as the first step. I can send separate PR to delete codestyle-intellj.xml and update codestyle-eclipse.xml if need be (create a separate issue #6790 to track it).

Jackie-Jiang · 2021-04-14T00:36:38Z

@mqliang Yes I totally agree maintaining one file is easier and more desired. I'm not sure if eclipse style tracks all the available options in the intellij style. If not, let's try to keep the current style as much as possible because most of the files already follow the format. When enabling the plugin, only the files that do not follow the current style should be changed.

Jackie-Jiang

Can we change the import rule to have static import after the regular import? We should keep the current order

mqliang · 2021-04-16T05:06:51Z

@Jackie-Jiang

Can we change the import rule to have static import after the regular import? We should keep the current order

Specified the import order as: java, javax, org, com, other unspecified, static import.

Jackie-Jiang · 2021-04-16T05:17:32Z

@mqliang

Specified the import order as: java, javax, org, com, other unspecified, static import.

I think we are following alphabetical order on regular import, followed by static import

… import

mqliang · 2021-04-16T05:33:00Z

@Jackie-Jiang

I think we are following alphabetical order on regular import, followed by static import

Done. Now all imports are split into two section: regular import in alphabetical order, static import

Jackie-Jiang

This is great! Well done!

mqliang mentioned this pull request Apr 13, 2021

Delete codestyle-intellj.xml and update codestyle-eclipse.xml, then enable eclipse formatter in spotless maven plugin #6790

Open

mqliang closed this Apr 14, 2021

mqliang reopened this Apr 14, 2021

mqliang closed this Apr 15, 2021

mqliang reopened this Apr 15, 2021

Jackie-Jiang reviewed Apr 15, 2021

View reviewed changes

Add spotless maven plugin to enforce java format.

385e0b7

mqliang added 2 commits April 15, 2021 22:28

Specify import order as: regular import in alphabetical order, static…

2fef408

… import

run mvn spotless:apply to format all java files

95158fb

Jackie-Jiang approved these changes Apr 16, 2021

View reviewed changes

Jackie-Jiang merged commit 7ce8b75 into apache:master Apr 16, 2021

mqliang deleted the fmt-enforce branch April 16, 2021 06:43

mqliang mentioned this pull request Apr 16, 2021

Add eclipse formatter in maven spotless plugin #6807

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add spotless maven plugin to enforce java format. #6782

Add spotless maven plugin to enforce java format. #6782

mqliang commented Apr 13, 2021 •

edited

Loading

mqliang commented Apr 13, 2021 •

edited

Loading

mqliang commented Apr 13, 2021

codecov-io commented Apr 13, 2021

Jackie-Jiang commented Apr 13, 2021

mqliang commented Apr 13, 2021 •

edited

Loading

Jackie-Jiang commented Apr 14, 2021

Jackie-Jiang left a comment

mqliang commented Apr 16, 2021 •

edited

Loading

Jackie-Jiang commented Apr 16, 2021

mqliang commented Apr 16, 2021

Jackie-Jiang left a comment

Add spotless maven plugin to enforce java format. #6782

Add spotless maven plugin to enforce java format. #6782

Conversation

mqliang commented Apr 13, 2021 • edited Loading

Description

Upgrade Notes

Release Notes

Documentation

mqliang commented Apr 13, 2021 • edited Loading

mqliang commented Apr 13, 2021

codecov-io commented Apr 13, 2021

Codecov Report

Jackie-Jiang commented Apr 13, 2021

mqliang commented Apr 13, 2021 • edited Loading

Jackie-Jiang commented Apr 14, 2021

Jackie-Jiang left a comment

Choose a reason for hiding this comment

mqliang commented Apr 16, 2021 • edited Loading

Jackie-Jiang commented Apr 16, 2021

mqliang commented Apr 16, 2021

Jackie-Jiang left a comment

Choose a reason for hiding this comment

mqliang commented Apr 13, 2021 •

edited

Loading

mqliang commented Apr 13, 2021 •

edited

Loading

mqliang commented Apr 13, 2021 •

edited

Loading

mqliang commented Apr 16, 2021 •

edited

Loading