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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
