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-16896 upgrade spotless version after we drop JDK8 #17423

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

m1a2st
Copy link
Contributor

@m1a2st m1a2st commented Oct 9, 2024

Jira: https://issues.apache.org/jira/browse/KAFKA-16896
update spotless version to 6.25.0 because we drop jdk8 in CI

Committer Checklist (excluded from commit message)

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

@github-actions github-actions bot added build Gradle build or GitHub Actions small Small PRs labels Oct 9, 2024
Copy link
Contributor

@mingyen066 mingyen066 left a comment

Choose a reason for hiding this comment

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

Thanks @m1a2st! I left some questions, PTAL.

build.gradle Outdated
@@ -2832,7 +2824,7 @@ project(':streams:streams-scala') {
dependsOn 'copyDependantLibs'
}

// spotless 6.14 requires Java 11 at runtime
// spotless 6.25 requires Java 11 at runtime
if (JavaVersion.current().isJava11Compatible()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also OR Java 17 in this if condition?

Copy link
Member

Choose a reason for hiding this comment

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

this check is unnecessary since we assume the JDK 8 is dropped

Comment on lines -857 to +859
// 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
if ((JavaVersion.current().isJava11() || (JavaVersion.current() == JavaVersion.VERSION_17))) {
apply plugin: 'com.diffplug.spotless'
spotless {
java {
targetExclude('**/generated/**/*.java','**/generated-test/**/*.java')
importOrder('kafka', 'org.apache.kafka', 'com', 'net', 'org', 'java', 'javax', '', '\\#')
removeUnusedImports()
}
apply plugin: 'com.diffplug.spotless'
spotless {
java {
targetExclude('**/generated/**/*.java','**/generated-test/**/*.java')
importOrder('kafka', 'org.apache.kafka', 'com', 'net', 'org', 'java', 'javax', '', '\\#')
removeUnusedImports()
Copy link
Contributor

@mingyen066 mingyen066 Oct 9, 2024

Choose a reason for hiding this comment

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

I think the limitation written in comment is still their. Can we use removeUnusedImports in JDK 21 already?

Copy link
Member

Choose a reason for hiding this comment

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

I think the limitation written in comment is still their. Can we use removeUnusedImports in JDK 21 already?

yes, I have tested it on my local. works well

@m1a2st
Copy link
Contributor Author

m1a2st commented Oct 9, 2024

@chia7712, @mingyen066 Thanks for your comments, I have been addressed by comments

@chia7712 chia7712 merged commit 91390a9 into apache:trunk Oct 9, 2024
6 checks passed
@cadonna
Copy link
Member

cadonna commented Oct 9, 2024

@m1a2st @chia7712 Have we already dropped JDK8? I still see minJavaVersion = 8, sourceCompatibility = minJavaVersion, and targetCompatibility = minJavaVersionin in build.gradle. These variables should be changed to implement KIP-750. Should we not have had implemented KIP-750 before merging this PR?
This breaks builds with java 8.

@frankvicky
Copy link
Contributor

Hi @cadonna,
I have opened a PR to address the issue you mentioned.
#17426

@chia7712
Copy link
Member

chia7712 commented Oct 9, 2024

@cadonna @cadonna, thanks for your response. We are planning to drop JDK 8, but we need this PR first since we need CI builds under JDK 21. However, spotless can’t run with JDK 21, which means our CI has been running without a spotless check for some time, making build errors more likely.

I’ve created tasks for the JDK 8 removal. @frankvicky is working on build.gradle and code cleanup, @Yunyung will update the E2E JDK and clean up old and unsupported Kafka versions, and @m1a2st is going to update the documentation for JDK 8. You can find more details in KAFKA-12894.

In short, we’re getting ready to say goodbye to JDK 8 😃.

tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Gradle build or GitHub Actions small Small PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants