refactor: disambiguate method names and overloads - #19825
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens static analysis and reduces ambiguity caused by confusingly similar method names/overloads across Druid’s Java codebase, primarily by enabling additional PMD checks and renaming methods/tests to be more semantically explicit.
Changes:
- Enables PMD’s
MissingOverriderule and adds a custom PMD XPath rule to prevent same-class methods differing only by capitalization (excluding inherited overrides). - Renames ambiguous/private/test helper methods to more descriptive names to avoid accidental overload binding.
- Renames cache configuration tests that previously differed only by capitalization, and adjusts a task-runner test expectation for blacklisted capacity reporting.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/src/test/java/org/apache/druid/client/cache/CacheConfigTest.java | Renames FALSE-related tests to avoid capitalization-only name differences. |
| processing/src/test/java/org/apache/druid/query/operator/NaiveSortOperatorTest.java | Renames helper to clarify it builds RACs from array inputs and updates callers. |
| processing/src/test/java/org/apache/druid/math/expr/ParserTest.java | Renames scalar constant helper to clarify overload intent and updates scalar call sites. |
| processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByQueryEngine.java | Renames private overload targets to make int-grouper-specific aggregation explicit. |
| processing/src/main/java/org/apache/druid/query/expression/NestedDataExpressions.java | Renames ExprEval-unwrapping helper and updates call sites for clearer intent. |
| indexing-service/src/test/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunnerTest.java | Adds an assertion that blacklisted task-slot counts are empty in the exercised scenario. |
| indexing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java | Renames worker-info accessor to avoid confusing capitalization and updates internal usage. |
| extensions-core/kafka-indexing-service/src/test/java/org/apache/druid/indexing/kafka/supervisor/KafkaSupervisorTest.java | Renames a test helper to clarify it derives tasks from supervisor tuning config. |
| codestyle/pmd-ruleset.xml | Adds MissingOverride and a custom rule preventing case-only method-name differences within a class. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 9 of 9 changed files, including surrounding callers and tests. Found one P2 public API compatibility issue; the NestedDataExpressions rename is behavior-preserving.
This is an automated review by Codex GPT-5.6-Sol
| } | ||
|
|
||
| public Collection<ImmutableWorkerInfo> getBlackListedWorkers() | ||
| public Collection<ImmutableWorkerInfo> getBlacklistedWorkerInfos() |
There was a problem hiding this comment.
P2 Preserve the existing public getBlackListedWorkers API
This removes the public getBlackListedWorkers() method and replaces it with getBlacklistedWorkerInfos(). Downstream extensions or tests may fail to compile, and existing binaries may hit NoSuchMethodError. Keep the old method as a deprecated delegating alias or explicitly document this incompatible API change.
There was a problem hiding this comment.
Fixed in afbbe686eb and pushed. Restored getBlackListedWorkers() as a deprecated delegating alias to getBlacklistedWorkerInfos(), preserving source and binary compatibility while retaining the new semantic name. Focused HttpRemoteTaskRunnerTest passed (21 tests), and Maven Checkstyle/PMD validation passed. Reviewed 9 of 9 changed files.
Created by the GPT-5.6-Sol model.
Summary
MissingOverriderule in the normal Maven static-check lifecycleHttpRemoteTaskRunnerworker-info accessor caught by that ruleMost missing-override findings in the broader CodeQL pattern are generated sources. The only handwritten
IntSet.clone()finding is not a valid Java override and cannot accept@Override. Required Servlet/interface overloads and compatibility APIs are intentionally preserved.Verification
mvn -ntp -B pmd:check -Dweb.console.skip=true -DskipTests -T1Cmvn -ntp -B checkstyle:check -pl processing,server,indexing-service,extensions-core/kafka-indexing-service -Dweb.console.skip=true -DskipTestsmvn -ntp -B test-compile -pl processing,server,indexing-service,extensions-core/kafka-indexing-service -am -Dweb.console.skip=true -DskipTests -T1C