ravikalla commented on code in PR #20219: URL: https://github.com/apache/kafka/pull/20219#discussion_r2231508798
########## connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java: ########## @@ -2998,15 +2993,6 @@ private void verifyVersionedTaskConverterFromWorker(String converterClassConfig, verify(plugins).newConverter(any(WorkerConfig.class), eq(converterClassConfig), eq(converterVersionConfig)); } - private void mockTaskHeaderConverter(ClassLoaderUsage classLoaderUsage, HeaderConverter returning) { Review Comment: Several private test helper methods like 'mockTaskConverter', 'mockTaskHeaderConverter', and 'autoCreateTopicsConfigs' were removed. While these appear unused now, they might be valuable test utilities. Need to verify: 1. Were these methods recently added for upcoming features? 2. Could they be useful for future test cases? 3. Should they be preserved but marked with @SuppressWarnings("unused") if they're intentionally kept for future use? ########## README.md: ########## @@ -201,7 +201,12 @@ For experiments (or regression testing purposes) add `-PcheckstyleVersion=X.y.z` #### Spotless #### The import order is a part of static check. please call `spotlessApply` to optimize the imports of Java codes before filing pull request. - ./gradlew spotlessApply +`./gradlew spotlessApply` + +#### Rewrite #### +The import order is a part of static check. please call `rewriteRun` to optimize the imports of Java codes before filing pull request. + +`./gradlew rewriteRun` Review Comment: "The import order is a part of static check" appears to be copy-pasted from the Spotless section. The Rewrite plugin does much more than just import ordering - it removes unused code, fields, and methods. This documentation should accurately describe what rewriteRun actually does. ########## build.gradle: ########## Review Comment: Both Spotless and Rewrite are configured to handle imports: Spotless has importOrder() configuration and Rewrite has RemoveUnusedImports recipe active. This creates overlapping responsibilities and potential conflicts. Consider either: 1. Using only Spotless for formatting/imports and Rewrite for code cleanup 2. Disabling import handling in one of the tools ########## gradle.properties: ########## @@ -14,6 +14,7 @@ # limitations under the License. group=org.apache.kafka +org.gradle.caching=true Review Comment: This enables Gradle's build cache. This is a significant change that affects build behavior and should be documented in the PR description as it affects all developers. Also, add some documentation on cache invalidation and troubleshooting. Also, see if local and/or remote caching should be configured aswell. ########## build.gradle: ########## @@ -224,6 +288,11 @@ allprojects { delete "${projectDir}/src/generated" delete "${projectDir}/src/generated-test" } + + // off switch for release: ' -x check' or ' -x rewriteDryRun spotlessJavaCheck' + tasks { Review Comment: This is to check task dependencies. But this will fail builds if any of the rewrite recipes detect issues. Also, the failOnDryRunResults = true setting means CI will break for any unused code and this is a breaking change for developers who might have unused private methods in their working branches. Consider making this opt-in initially or only enabled in CI. ########## build.gradle: ########## @@ -16,6 +16,11 @@ import org.ajoberstar.grgit.Grgit import java.nio.charset.StandardCharsets +import static java.lang.Boolean.valueOf +import static java.lang.System.getenv +import static java.lang.System.getenv Review Comment: Duplicate import ########## build.gradle: ########## @@ -29,18 +34,77 @@ buildscript { } plugins { - id 'com.github.ben-manes.versions' version '0.48.0' - id 'idea' - id 'jacoco' - id 'java-library' - id 'org.owasp.dependencycheck' version '8.2.1' - id 'org.nosphere.apache.rat' version "0.8.1" + id "com.diffplug.spotless" version "7.2.1" + id "com.github.ben-manes.versions" version "0.48.0" + id "com.github.spotbugs" version "6.0.25" apply false + id "com.gradleup.shadow" version "8.3.6" apply false + id "idea" id "io.swagger.core.v3.swagger-gradle-plugin" version "${swaggerVersion}" + id "jacoco" + id "java-library" + id "org.nosphere.apache.rat" version "0.8.1" + id "org.openrewrite.rewrite" version "7.12.1" + id "org.owasp.dependencycheck" version "8.2.1" + id "org.scoverage" version "8.0.3" apply false +} + +rewrite { + activeRecipe( + "org.openrewrite.gradle.GradleBestPractices", + "org.openrewrite.java.RemoveUnusedImports", + "org.openrewrite.staticanalysis.RemoveUnusedLocalVariables", + "org.openrewrite.staticanalysis.RemoveUnusedPrivateFields", + "org.openrewrite.staticanalysis.RemoveUnusedPrivateMethods", + //"org.openrewrite.java.OrderImports", + //"org.openrewrite.java.format.RemoveTrailingWhitespace", + //"org.openrewrite.java.migrate.UpgradeToJava17", + //"org.openrewrite.java.migrate.util.MigrateCollectionsSingletonList", + //"org.openrewrite.java.migrate.util.MigrateCollectionsUnmodifiableList", + //"org.openrewrite.java.recipes.JavaRecipeBestPractices", + //"org.openrewrite.java.recipes.RecipeNullabilityBestPractices", + //"org.openrewrite.java.recipes.RecipeTestingBestPractices", + //"org.openrewrite.staticanalysis.CodeCleanup", + //"org.openrewrite.staticanalysis.EmptyBlock", + //"org.openrewrite.staticanalysis.EqualsAvoidsNull", + //"org.openrewrite.staticanalysis.JavaApiBestPractices", + //"org.openrewrite.staticanalysis.MissingOverrideAnnotation", + //"org.openrewrite.staticanalysis.ModifierOrder", + //"org.openrewrite.staticanalysis.NoFinalizer", + //"org.openrewrite.staticanalysis.RemoveCallsToSystemGc", + //"org.openrewrite.staticanalysis.RemoveUnneededAssertion", + //"org.openrewrite.staticanalysis.StringLiteralEquality", + //"org.openrewrite.staticanalysis.UnnecessaryParentheses", + //"org.openrewrite.staticanalysis.UnnecessaryThrows", + //"org.openrewrite.text.EndOfLineAtEndOfFile", + //"tech.picnic.errorprone.refasterrules.CollectionRulesRecipes", + //"tech.picnic.errorprone.refasterrules.FileRulesRecipes", + //"tech.picnic.errorprone.refasterrules.NullRulesRecipes", + //"tech.picnic.errorprone.refasterrules.NullRulesRecipes", + //"tech.picnic.errorprone.refasterrules.StreamRulesRecipes", + //"tech.picnic.errorprone.refasterrules.StringRulesRecipes", + // running active recipes: tech.picnic.errorprone.refasterrules.NullRulesRecipes + //<============-> 99% EXECUTING [1h 38s] Review Comment: As this recipe took over 1.5 hours to run, this is concerning for developer productivity. Careful evaluation is required as it is taking very long - I would get it reviewed by someone else. ########## build.gradle: ########## @@ -16,6 +16,11 @@ import org.ajoberstar.grgit.Grgit import java.nio.charset.StandardCharsets +import static java.lang.Boolean.valueOf Review Comment: Unused import ########## build.gradle: ########## @@ -16,6 +16,11 @@ import org.ajoberstar.grgit.Grgit import java.nio.charset.StandardCharsets +import static java.lang.Boolean.valueOf +import static java.lang.System.getenv +import static java.lang.System.getenv +import static java.util.Objects.isNull Review Comment: Unused import -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org