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.

Sign up for GitHub

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

Jump to bottom

KAFKA-10787: Update spotless version and remove support JDK8 #16176

Merged
merged 18 commits into from
Jun 5, 2024
Merged

KAFKA-10787: Update spotless version and remove support JDK8 #16176

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1edc40e
update spotless version
gongxuanzhang Jun 3, 2024
ce9cbec
Merge branch 'trunk' into update-spotless-version
gongxuanzhang Jun 4, 2024
911fa3f
update include to exclude
gongxuanzhang Jun 4, 2024
fa9f002
update name to path
gongxuanzhang Jun 4, 2024
9453039
update exclude module name to path
gongxuanzhang Jun 4, 2024
efe7050
update comment
gongxuanzhang Jun 4, 2024
ca30c32
Reduced version test ci
gongxuanzhang Jun 4, 2024
a39d07b
Merge branch 'apache:trunk' into update-spotless-version
gongxuanzhang Jun 4, 2024
854aa32
update version and add comment
gongxuanzhang Jun 4, 2024
95d58e4
Merge branch 'apache:trunk' into update-spotless-version
gongxuanzhang Jun 4, 2024
abc921e
remove verbose word
gongxuanzhang Jun 4, 2024
a015ce6
move commit comment
gongxuanzhang Jun 4, 2024
5524a5e
Merge branch 'apache:trunk' into update-spotless-version
gongxuanzhang Jun 4, 2024
4c95e24
Merge branch 'apache:trunk' into update-spotless-version
gongxuanzhang Jun 4, 2024
b5de922
Update README.md
gongxuanzhang Jun 4, 2024
16249c1
Merge branch 'apache:trunk' into update-spotless-version
gongxuanzhang Jun 5, 2024
75ede30
add future module
gongxuanzhang Jun 5, 2024
0f108eb
Merge branch 'apache:trunk' into update-spotless-version
gongxuanzhang Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ If needed, you can specify the Scala version with `-PscalaVersion=2.13`.
There are two code quality analysis tools that we regularly run, spotbugs and checkstyle.

#### Checkstyle ####
Checkstyle enforces a consistent coding style in Kafka.
Checkstyle enforces a consistent coding style in Kafka. Make sure you pass it before you submit.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remind developers that spotlessCheck will fail if env is JDK8. Make sure you pass it before you submit. is a bit verbose since we have QA :)

You can run checkstyle using:

./gradlew checkstyleMain checkstyleTest spotlessCheck
Expand All @@ -233,7 +233,7 @@ The checkstyle warnings will be found in `reports/checkstyle/reports/main.html`
subproject build directories. They are also printed to the console. The build will fail if Checkstyle fails.

#### Spotless ####
The import order is a part of static check. please call `spotlessApply` to optimize the imports of Java codes before filing pull request :
The import order is a part of static check. please call `spotlessApply` (require JDK 11+) to optimize the imports of Java codes before filing pull request.

./gradlew spotlessApply

Expand Down
66 changes: 60 additions & 6 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,10 @@ plugins {
// Updating the shadow plugin version to 8.1.1 causes issue with signing and publishing the shadowed
// artifacts - see https://github.com/johnrengelman/shadow/issues/901
id 'com.github.johnrengelman.shadow' version '8.1.0' apply false
// the minimum required JRE of 6.14.0+ is 11
chia7712 marked this conversation as resolved.
Show resolved Hide resolved
// refer:https://github.com/diffplug/spotless/tree/main/plugin-gradle#requirements
id 'com.diffplug.spotless' version "6.13.0" apply false
// Spotless 6.13.0 has issue with Java 21 (see https://github.com/diffplug/spotless/pull/1920), and Spotless 6.14.0+ requires JRE 11
// We are going to drop JDK8 support. Hence, the spotless is upgrade to newest version and be applied only if the build env is compatible with JDK 11.
// spotless 6.25.0 has issue in runtime with JDK8 even through we define it with `apply:false`. see https://github.com/diffplug/spotless/issues/2156 for more details
id 'com.diffplug.spotless' version "6.14.0" apply false
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you give a try for 6.24.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

if 6.14.0 is the latest version which can work with JDK8 (as you described in the comment), please change the "spotless 6.25.0" to "spotless 6.15+"

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
id 'com.diffplug.spotless' version "6.14.0" apply false
id 'com.diffplug.spotless' version "6.25.0" apply false

Copy link
Member

Choose a reason for hiding this comment

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

You can always use the latest Spotless and bump your JRE to match it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Goooler thanks for your comment.

Kafka still support JDK 8, so we make CI run checks/tests with JDK8. However, as @gongxuanzhang described, spotless 6.15+ can't work with JDK8 even though we set "apply false".

Copy link
Member

Choose a reason for hiding this comment

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

If you want the binary compatibility to keep on Java 8, just declare sourceCompatibility and so on.

See also https://jakewharton.com/gradle-toolchains-are-rarely-a-good-idea/

Copy link
Contributor

@chia7712 chia7712 Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yep, you are right. We have set the sourceCompatibility=8

https://github.com/apache/kafka/blob/trunk/build.gradle#L57
https://github.com/apache/kafka/blob/trunk/build.gradle#L318

I know that we can use JDK11 to build kafak which is compatible with JRE8. However, our rules is "we ought to run all checks/tests with all supported JDKs". That is why we need to make sure spotless can work with all supported JDKs.

for another, we will drop JDK8 in next release. Hence, we can remove this workaround and upgrade spotless to newest version later.

Copy link
Member

Choose a reason for hiding this comment

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

Good to see.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Goooler Do you approve this PR? it would be great to get approve from spotless core developer :)

Copy link
Member

@Goooler Goooler Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the delay.

I think it would be great to throw exceptions if the current JRE is not Java 11 compatible. See #16176 (comment).

Either way, we can do that after dropping JDK8 as you described.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be great to throw exceptions if the current JRE is not Java 11 compatible.

I had similar idea before (see #13311 (comment)) . However, that means our developers can't use JDK8 to write code for kafka. Also, we can't follow our JDK rules - run all checks/tests with all supported JDKs

}

ext {
Expand Down Expand Up @@ -200,7 +201,60 @@ def determineCommitId() {
}
}

def spotlessApplyModules = ['']
def excludedSpotlessModules = [':clients',
Copy link
Contributor

Choose a reason for hiding this comment

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

noticed that #16198 will add a new module. Maybe you can include that in this PR even though #16198 is not merged yet?

':connect:api',
':connect:basic-auth-extension',
':connect:file',
':connect:json',
':connect:mirror',
':connect:mirror-client',
':connect:runtime',
':connect:test-plugins',
':connect:transforms',
':core',
':examples',
':generator',
':group-coordinator',
':jmh-benchmarks',
':log4j-appender',
':metadata',
':raft',
':server',
':server-common',
':shell',
':storage',
':storage:storage-api', // rename in settings.gradle
':streams',
':streams:examples',
':streams:streams-scala',
':streams:test-utils',
':streams:upgrade-system-tests-0100',
':streams:upgrade-system-tests-0101',
':streams:upgrade-system-tests-0102',
':streams:upgrade-system-tests-0110',
':streams:upgrade-system-tests-10',
':streams:upgrade-system-tests-11',
':streams:upgrade-system-tests-20',
':streams:upgrade-system-tests-21',
':streams:upgrade-system-tests-22',
':streams:upgrade-system-tests-23',
':streams:upgrade-system-tests-24',
':streams:upgrade-system-tests-25',
':streams:upgrade-system-tests-26',
':streams:upgrade-system-tests-27',
':streams:upgrade-system-tests-28',
':streams:upgrade-system-tests-30',
':streams:upgrade-system-tests-31',
':streams:upgrade-system-tests-32',
':streams:upgrade-system-tests-33',
':streams:upgrade-system-tests-34',
':streams:upgrade-system-tests-35',
':streams:upgrade-system-tests-36',
':streams:upgrade-system-tests-37',
':tools',
':tools:tools-api',
':transaction-coordinator',
':trogdor']


apply from: file('wrapper.gradle')
Expand Down Expand Up @@ -798,8 +852,8 @@ subprojects {
skipProjects = [ ":jmh-benchmarks", ":trogdor" ]
skipConfigurations = [ "zinc" ]
}

if (project.path in spotlessApplyModules) {
if(JavaVersion.current().isJava11Compatible() && project.path !in excludedSpotlessModules) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can just throw exceptions if the current JRE is not Java 11 compatible.

apply plugin: 'com.diffplug.spotless'
Copy link
Contributor

Choose a reason for hiding this comment

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

another idea: Could we revers the spotlessApplyModules to be the "exclude list"? For example:

def excludedSpotlessModules = ['core', 'client']

and then we remove module one by one if we apply the spotless rule. It is more clear then before, since we can "see" which module is not applied. 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.

Yes, good idea.

spotless {
java {
Expand Down