Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-10787: Apply spotless to transaction-coordinator and server-common #16172

Merged

Conversation

gongxuanzhang
Copy link
Contributor

@gongxuanzhang gongxuanzhang commented Jun 3, 2024

Brief Introduction

This PR is sub PR from #16097.
That Pr we add spotless plugin to build.gradle.
In order not to modify large-scale code, we plan apply it step by step in module.
The first is 'transaction-coordinator' 'server-common'.
Module 'transaction-coordinator' meets the requirements(only have 2 Java class) so apply it directly.
This PR update 'server-common' module code review import order.
We will apply spotless to all module in future

How to test:

We can run ./gradlew spotlessCheck check for code that does not meet requirements.
If we get report that error , we can run ./gradlew spotlessApply to review my code.
In this PR, all change(exclude build.gradle) byspotlessApply`

@gongxuanzhang gongxuanzhang changed the title KAFKA-10787: Add import ordering checkstyle rule and configure an automatic formatter KAFKA-10787: Add spotless into transaction-coordinator and server-common Jun 3, 2024
@gongxuanzhang gongxuanzhang changed the title KAFKA-10787: Add spotless into transaction-coordinator and server-common KAFKA-10787: Apply spotless to transaction-coordinator and server-common Jun 3, 2024
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chia7712
Copy link
Member

chia7712 commented Jun 3, 2024

@gongxuanzhang Could you please update the description to share the tips of "how to test this PR"?

@gongxuanzhang
Copy link
Contributor Author

@chia7712 I changed it , plz take a look

@chia7712
Copy link
Member

chia7712 commented Jun 3, 2024

@gongxuanzhang Could you please check the build error?

@chia7712
Copy link
Member

chia7712 commented Jun 3, 2024

It seems we are facing Trolley problem :(

  1. we use spotless 6.13 in order to support JDK 8
  2. the google-format version used by spotless 6.13 has issue in JDK 21 (Fix Java 21 compatibilities diffplug/spotless#1920)

Given that we are going to get rid of JDK 8 and there is example of ignoring JDK8
https://github.com/apache/kafka/blob/trunk/build.gradle#L2448

Maybe we can remove the support of JDK8 from spotless!

  1. upgrade spotless to newest version
  2. add JavaVersion.current().isJava11Compatible() check to apply spotless only if the env is JDK11+

@gongxuanzhang WDYT?

@gongxuanzhang
Copy link
Contributor Author

I agree remove support JDK8.
I will change spotless version and comment

@chia7712
Copy link
Member

chia7712 commented Jun 4, 2024

@gongxuanzhang please fix the conflicts

@gongxuanzhang
Copy link
Contributor Author

@chia7712 complete

@gongxuanzhang gongxuanzhang force-pushed the import-order-transaction-coordinator branch from 9fb6d4e to f2aafcc Compare June 5, 2024 14:48
@gongxuanzhang gongxuanzhang reopened this Jun 5, 2024
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gongxuanzhang Could you please rebase code to trigger QA again?

build.gradle Outdated
@@ -853,8 +851,8 @@ subprojects {
skipProjects = [ ":jmh-benchmarks", ":trogdor" ]
skipConfigurations = [ "zinc" ]
}

if(JavaVersion.current().isJava11Compatible() && project.path !in excludedSpotlessModules) {
// current spotless work error in JDK21,upgrade spotless version when kafka drop support JDK8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the task `removeUnusedImports` is implemented by google-java-format, and unfortunately the google-java-format version used by spotless 6.14.0 can't work with JDK 21. Hence, we apply spotless tasks only if the env is either JDK11 or JDK17

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will update comment

@chia7712
Copy link
Member

chia7712 commented Jun 7, 2024

I have called QA again

@chia7712
Copy link
Member

chia7712 commented Jun 8, 2024

@gongxuanzhang could you please rebase code to include #16249

@gongxuanzhang
Copy link
Contributor Author

gongxuanzhang commented Jun 8, 2024

Okay, next we wait for CI @chia7712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants