KAFKA-12572: Add import ordering checkstyle rule and configure an automatic formatter #10428

dongjinleekr · 2021-03-29T11:51:13Z

This PR is a part of KAFKA-10787 umbrella task, replacing the previous implementation here. In short, this PR provides automatic import ordering and checking configurations, which will be a foundation of actual code formatting tasks later. Here are some backgrounds.

When I first started KAFKA-10787, I thought it is a trivial one and can be resolved with only one PR. But it is entirely wrong.

It modifies hundreds of files; it is not only hard to review but also, every time it has some code conflict with the trunk branch.
As @mjsax already pointed out, the previous implementation omits the essential way of solving this problem - automatic import formatter. Since it includes the checkstyle rules only, it shows error messages for the import ordering violation and that's it.

For the reasons above, it is almost impossible to resolve this issue with one commit or PR.

So, I restarted this feature from scratch. First of all, I changed the approach like following:

Take an incremental approach, i.e., enlarge the import ordering rules applied area gradually. (In other words, narrow down the exception area to the import ordering rules, step by step.)
By adopting automatic code formatter, make the build tool to apply the import order automatically.

This approach has the advantages like:

We can apply the import order step by step; it is feasible to review and greatly reduces the possibility of code conflict.
The dev team doesn't have to care about the formatting anymore; the build tool automatically take cares of it.

With this scheme, we can expand the scope of auto-formatting and checkstyle incrementally.

In turn, I converted KAFKA-10787 into an umbrella one. As the first subtask, this PR do the following:

Define the import ordering rule in checkstyle: kafka, org.apache.kafka, com, net, org, java, javax and static imports. It is identical to what we discussed in the dev mailing list, except each package is grouped independely. (e.g., kafka and org.apache.kafka are regarded as independent groups.)
Add an eclipse formatter file, which includes:
- Minimal formatting rules that follow current checkstyle rules.
- ImportOrder rule for the newly introduced import ordering rule.
Add a spotless configuration to reformat the subset of modules automatically.

Here are some reasons for the decisions I made:

Why choose eclipse formatter?

As all of you know, Apache Kafka is using gradle as its build tool. To automatically apply the import ordering rule with gradle, we have to use spotless, which supports two formatters - google formatter and eclipse formatter.

If we choose google formatter, it changes not only import ordering but also the other aspects of code, like an indentation. To minimize non-import ordering modifications, I choose eclipse formatter, which supports a custom formatter configuration.

As you can see here, the formatter defines a minimal rules for out code only.
The original proposal consists of 3 groups: kafka/org.apache.kafka, com/org/net, java/javax, and static imports. Why are those packages separated into independent groups?

It's simple - the spotless automatic formatter doesn't allow packages with different prefixes into one group. So I separated them.

* Updated (June 10th 2021): The core module is excluded from the PR, and it now only focuses on providing auto-formatting configurations.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

dongjinleekr · 2021-03-29T11:56:15Z

I think @ijuma would be the best reviewer for the new gradle configuration introduced here.

@hachikuji @mjsax @ableegoldman @cadonna Could you have a look? The automatic formatter formatted core module here. Have a try! 😃

dongjinleekr · 2021-04-12T11:40:34Z

@cadonna Hi Bruno, Congratulations on becoming a committer.

Could you have a look? 🙇

cadonna

@dongjinleekr Thank you for your perseverance with this!

I did a first pass. Here is my feedback.

README.md

eclipse-formatter.xml

core/src/main/scala/kafka/tools/StreamsResetter.java

eclipse-formatter.xml

core/src/main/scala/kafka/tools/StreamsResetter.java

eclipse-formatter.xml

dongjinleekr · 2021-04-14T15:25:44Z

Hi @cadonna,

Thanks again for your detailed review. Here is the update.

Following your suggestions, I tried some other configurations. First of all, there is no way to use automatic import ordering only - without any configuration, the formatter formats the code with its default configuration. So, we have to find the least-effecting configuration.

The major differences between the last draft are like the following:

Remove line wrapping rule for 120 characters.
Comment is not formatted anymore.
Remove bracing-related rules.

But, I kept the other rules, like:

As of present, both of 4 space indenting and 8 space indenting are co-existing in the code. I configured to use 4 space indenting for its majority.
Several styles for the parameter formatting for the class and method definition also exist in the code. It is configured to the least-effecting configuration.

Please have a look when you are free. Thanks again for the review!

+1. It would be better to check how the new configuration formats streams module. I tested with that module, for it has so many *.java files. 😉

cadonna

@dongjinleekr Thank you for the updates!

I left some comments. The comments about readability are quite subjective. Would be interesting to hear other opinions.

I would try to minimize changes of line breaks by the formatter, because I think most of the people add line breaks to make the code more readable. I have the impression that adding and removing line breaks is something that is hard to automate because it is often a case by case decision.

An exception to that are line breaks in method calls and method definitions. I think having a formatter rule for that would be great.

eclipse-formatter.xml

core/src/main/scala/kafka/tools/StreamsResetter.java

core/src/test/java/kafka/test/MockController.java

core/src/test/java/kafka/testkit/KafkaClusterTestKit.java

dongjinleekr · 2021-04-20T08:54:54Z

Hi @cadonna, Here is the fix.

Added a license header to eclipse-formatter.xml.
Re-add line break formatting with 200 characters. (It resolved lots of formatting issues!)

+1. in ClusterTestExtensions.java, a space is added between default and :, resulting default :; It seems like a bug in eclipse formatter. 😞

dongjinleekr · 2021-05-07T13:09:13Z

Rebased onto the latest trunk.

@cadonna Could wou have a look now? 😃

dongjinleekr · 2021-05-25T13:48:12Z

Rebased onto the latest trunk. cc/ @cadonna

cadonna

@dongjinleekr Thank you for the update!

I had just some minor comments.

LGTM!

But I think also somebody that contributed to the core module should look at this PR. @ijuma @hachikuji @cmccabe @rajinisivaram @omkreddy @dajac

eclipse-formatter.xml

core/src/test/java/kafka/test/junit/ClusterTestExtensions.java

dongjinleekr · 2021-05-27T04:16:57Z

@cadonna I greatly appreciate your detailed review. Here is the update! 😄

dejan2609 · 2021-06-02T21:59:02Z

Shameless plug and related to CheckStyle: ⏩ #10698 (needs a review / approval).

core/src/main/java/kafka/metrics/FilteringJmxReporter.java

dongjinleekr · 2021-06-03T12:13:01Z

@cadonna Here it is, with the following: 1. making server-common to ignore import orders. 2. rebasing onto the latest trunk.

+1. It seems like we can make the eclipse formatter do not insert a newline at the end of the class, but not removing it. (see here.) So I reformatted the core module from the latest trunk.

cadonna · 2021-06-03T19:03:59Z

core/src/test/java/kafka/test/MockController.java

        throw new UnsupportedOperationException();
    }

    @Override
    public CompletableFuture<Map<ConfigResource, ApiError>> legacyAlterConfigs(
-            Map<ConfigResource, Map<String, String>> newConfigs, boolean validateOnly) {
+                                                                               Map<ConfigResource, Map<String, String>> newConfigs, boolean validateOnly) {


One question here. If you put the parameters of this method on the same line as the method name and you run again the formatter, will the parameters stay on the same line as the method name?
What would happen if you put the first parameter on the same line as the method name and then put each of the remaining parameters on its line own line?

case 1. If you put the parameters of this method on the same line as the method name and run formatter: remains one line.
case 2. If you put the parameters separated and run formatter: broken. (below)

I do not know what you exactly mean with "broken", but that is the behavior I hoped for.

Okay. I will fix it with the second approach.

Could you put the first parameter in the same line as the method name and put the remaining parameters on their own line as I proposed? Could you do the same for incrementalAlterConfigs()?

If anybody does not like this style, we can still delete

<setting id="org.eclipse.jdt.core.formatter.alignment_for_parameters_in_constructor_declaration" value="18"/> <setting id="org.eclipse.jdt.core.formatter.alignment_for_parameters_in_method_declaration" value="18"/>

from the formatter configuration, right?

After that, I think, we can merge. Besides the import order changes, you kept the just a few formatting style changes, which I think is good. If somebody does not agree, we can make it even less invasive.

dongjinleekr · 2021-06-07T11:52:24Z

@cadonna Here it is. Rebased onto the latest trunk + manually formatted MockController#{legacyAlterConfigs, incrementalAlterConfigs}. I also reviewed the other files and it seems no more similar cases exist.

dongjinleekr · 2021-06-08T03:18:34Z

@cadonna Here is the update. I excluded the core module from this PR (i.e., it contains formatting configurations only, not actual formatting now) and opened separate issues:

As soon as this PR is merged, I start the issues above.

cadonna

@dongjinleekr Thank you for the updates and your patience!

I a few comments and I think you need to update the PR description.

build.gradle

README.md

dongjinleekr · 2021-06-10T12:42:58Z

@cadonna Here it is. Rebased onto the latest trunk and removed the formatting section from README.md.

dongjinleekr · 2021-06-21T07:04:18Z

Rebased onto the latest trunk. cc/ @cadonna

1. Define an Java import order: kafka, org.apache.kafka, com, net, org, java, javax, (static imports). 2. Add eclipse formatter and configure automatic code formatting. 3. Suppress import order check for all modules. 4. Separate raft module configurations in suppression.xml. 5. Add storage module configurations in suppression.xml. Define an Java import order and configure automatic code formatting (2) Make server-common's import ordering off

…Bracing related formatting rules. 3. Comment is not a target of the formatting anymore. 4. Wrap after assignment operator.

…ormatting with 200 characters. 3. Add trogdor to suppressions.xml (Trogdor is separated from tools in fc405d7)

…se.jdt.core.formatter.indent_empty_lines'. 2. Add 'org.eclipse.jdt.core.formatter.insert_space_before_colon_in_default' entry. (i.e., No space between switch statement's default case and colon.) 3. Remove 'org.eclipse.jdt.core.formatter.blank_lines_after_last_class_body_declaration' entry. (i.e., No blank line at the end of the class.)

…existing blank lines but just not adding ones.)

dongjinleekr · 2021-07-08T12:24:59Z

Rebased onto the latest trunk. Also, I reflected the recent project structure change (i.e., storage → storage/api) by updating suppressions.xml. cc/ @cadonna @ableegoldman

chia7712 · 2024-04-30T00:25:16Z

@dongjinleekr Do you have free time to fix the conflicts? I'd like to review it!

mimaison · 2024-10-15T19:19:35Z

This was done in 342e691, closing

dongjinleekr mentioned this pull request Mar 29, 2021

KAFKA-10787: Introduce an import order in Java sources #8404

Closed

dongjinleekr force-pushed the feature/KAFKA-12572 branch from 424bae7 to 3e5b6c3 Compare March 31, 2021 09:00

dongjinleekr force-pushed the feature/KAFKA-12572 branch 2 times, most recently from af84b33 to b8257b6 Compare April 12, 2021 10:39

cadonna reviewed Apr 13, 2021

View reviewed changes

dongjinleekr force-pushed the feature/KAFKA-12572 branch from b8257b6 to 39a0257 Compare April 14, 2021 15:14

cadonna reviewed Apr 16, 2021

View reviewed changes

dongjinleekr force-pushed the feature/KAFKA-12572 branch from 39a0257 to 4865c11 Compare April 20, 2021 08:50

dongjinleekr force-pushed the feature/KAFKA-12572 branch from 4865c11 to a9e9a34 Compare May 7, 2021 13:08

dongjinleekr force-pushed the feature/KAFKA-12572 branch from a9e9a34 to cafbd9e Compare May 25, 2021 13:47

cadonna approved these changes May 26, 2021

View reviewed changes

eclipse-formatter.xml Show resolved Hide resolved

eclipse-formatter.xml Outdated Show resolved Hide resolved

core/src/test/java/kafka/test/junit/ClusterTestExtensions.java Outdated Show resolved Hide resolved

cadonna mentioned this pull request May 31, 2021

KAFKA-8410: Migrating stateful operators to new Processor API #10507

Closed

3 tasks

cadonna reviewed Jun 3, 2021

View reviewed changes

core/src/main/java/kafka/metrics/FilteringJmxReporter.java Outdated Show resolved Hide resolved

dongjinleekr force-pushed the feature/KAFKA-12572 branch 3 times, most recently from 4d20bb3 to e179f92 Compare June 3, 2021 12:11

cadonna reviewed Jun 3, 2021

View reviewed changes

dongjinleekr force-pushed the feature/KAFKA-12572 branch 2 times, most recently from c250545 to b970d54 Compare June 8, 2021 03:14

cadonna reviewed Jun 9, 2021

View reviewed changes

build.gradle Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

dongjinleekr force-pushed the feature/KAFKA-12572 branch from b970d54 to a98f1fb Compare June 10, 2021 12:41

dongjinleekr force-pushed the feature/KAFKA-12572 branch from a98f1fb to cb67573 Compare June 21, 2021 07:04

dongjinleekr added 6 commits July 8, 2021 16:11

1. Fix typo: working in progress → work in progress. 2. Remove Line, …

2ef5af7

…Bracing related formatting rules. 3. Comment is not a target of the formatting anymore. 4. Wrap after assignment operator.

1. Add license header to eclipse-formatter.xml 2. Re-add line break f…

8de2a0f

…ormatting with 200 characters. 3. Add trogdor to suppressions.xml (Trogdor is separated from tools in fc405d7)

Make no blank line at the end of the class (note: it does not remove …

a7a2a7a

…existing blank lines but just not adding ones.)

Remove the formatting section from README.md

f7865fb

dongjinleekr force-pushed the feature/KAFKA-12572 branch from cb67573 to f7865fb Compare July 8, 2021 07:11

Suppress storage/api module

5d26cf1

dongjinleekr mentioned this pull request Jul 8, 2022

KAFKA-7632: Support Compression Level #10826

Closed

3 tasks

chia7712 mentioned this pull request May 10, 2024

KAFKA-16643 Add ModifierOrder checkstyle rule #15890

Merged

gongxuanzhang mentioned this pull request May 28, 2024

KAFKA-10787: Add import ordering checkstyle rule and configure an automatic formatter #16097

Merged

mimaison closed this Oct 15, 2024

cadonna mentioned this pull request Jan 20, 2025

KAFKA-18311: Internal Topic Manager (5/5) #18442

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-12572: Add import ordering checkstyle rule and configure an automatic formatter #10428

KAFKA-12572: Add import ordering checkstyle rule and configure an automatic formatter #10428

dongjinleekr commented Mar 29, 2021 •

edited

Loading

dongjinleekr commented Mar 29, 2021

dongjinleekr commented Apr 12, 2021

cadonna left a comment

dongjinleekr commented Apr 14, 2021

cadonna left a comment

dongjinleekr commented Apr 20, 2021

dongjinleekr commented May 7, 2021 •

edited

Loading

dongjinleekr commented May 25, 2021

cadonna left a comment

dongjinleekr commented May 27, 2021

dejan2609 commented Jun 2, 2021

dongjinleekr commented Jun 3, 2021

cadonna Jun 3, 2021

dongjinleekr Jun 6, 2021

cadonna Jun 7, 2021

dongjinleekr Jun 7, 2021

cadonna Jun 7, 2021

cadonna Jun 7, 2021

dongjinleekr commented Jun 7, 2021

dongjinleekr commented Jun 8, 2021

cadonna left a comment

dongjinleekr commented Jun 10, 2021

dongjinleekr commented Jun 21, 2021

dongjinleekr commented Jul 8, 2021

chia7712 commented Apr 30, 2024

mimaison commented Oct 15, 2024

KAFKA-12572: Add import ordering checkstyle rule and configure an automatic formatter #10428

KAFKA-12572: Add import ordering checkstyle rule and configure an automatic formatter #10428

Conversation

dongjinleekr commented Mar 29, 2021 • edited Loading

Committer Checklist (excluded from commit message)

dongjinleekr commented Mar 29, 2021

dongjinleekr commented Apr 12, 2021

cadonna left a comment

Choose a reason for hiding this comment

dongjinleekr commented Apr 14, 2021

cadonna left a comment

Choose a reason for hiding this comment

dongjinleekr commented Apr 20, 2021

dongjinleekr commented May 7, 2021 • edited Loading

dongjinleekr commented May 25, 2021

cadonna left a comment

Choose a reason for hiding this comment

dongjinleekr commented May 27, 2021

dejan2609 commented Jun 2, 2021

dongjinleekr commented Jun 3, 2021

cadonna Jun 3, 2021

Choose a reason for hiding this comment

dongjinleekr Jun 6, 2021

Choose a reason for hiding this comment

cadonna Jun 7, 2021

Choose a reason for hiding this comment

dongjinleekr Jun 7, 2021

Choose a reason for hiding this comment

cadonna Jun 7, 2021

Choose a reason for hiding this comment

cadonna Jun 7, 2021

Choose a reason for hiding this comment

dongjinleekr commented Jun 7, 2021

dongjinleekr commented Jun 8, 2021

cadonna left a comment

Choose a reason for hiding this comment

dongjinleekr commented Jun 10, 2021

dongjinleekr commented Jun 21, 2021

dongjinleekr commented Jul 8, 2021

chia7712 commented Apr 30, 2024

mimaison commented Oct 15, 2024

dongjinleekr commented Mar 29, 2021 •

edited

Loading

dongjinleekr commented May 7, 2021 •

edited

Loading