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

Reply via email to