build: detect unused parameters and local variables - #19821
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens the Java static-check gate by enabling PMD’s unused-parameter / unused-local checks, then updates production code to comply by removing genuinely unused parameters/locals and adopting an ignored* naming convention for intentionally unused try-with-resources handles.
Changes:
- Enabled PMD
UnusedFormalParameterandUnusedLocalVariablerules incodestyle/pmd-ruleset.xml. - Removed unused private-method parameters and dead locals, updating affected call sites.
- Renamed intentionally-unused try-with-resources variables to
ignored*to satisfy PMD while preserving close semantics.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| sql/src/main/java/org/apache/druid/sql/calcite/schema/InformationSchema.java | Removes unused parameter from column-metadata helper and updates callers. |
| sql/src/main/java/org/apache/druid/sql/calcite/rule/logical/UnnestInputCleanupRule.java | Removes unused constructor parameter in RexShuttle helper and updates instantiation. |
| sql/src/main/java/org/apache/druid/sql/avatica/DruidAvaticaProtobufHandler.java | Renames unused try-with-resources handle to ignored*. |
| sql/src/main/java/org/apache/druid/sql/avatica/DruidAvaticaJsonHandler.java | Renames unused try-with-resources handle to ignored*. |
| server/src/main/java/org/apache/druid/server/coordinator/loading/StrategicSegmentAssigner.java | Removes unused parameter from replica-drop helper and updates callers. |
| server/src/main/java/org/apache/druid/segment/realtime/appenderator/StreamAppenderator.java | Removes unused parameter from sink-overhead calculation and updates callers. |
| processing/src/main/java/org/apache/druid/java/util/common/guava/ConcatSequence.java | Renames unused try-with-resources handle to ignored*. |
| processing/src/main/java/org/apache/druid/java/util/common/FileUtils.java | Renames unused try-with-resources handle to ignored*. |
| processing/src/main/java/org/apache/druid/frame/processor/FrameProcessors.java | Renames unused try-with-resources handles to ignored*. |
| processing/src/main/java/org/apache/druid/common/utils/SocketUtil.java | Renames unused try-with-resources handle to ignored*. |
| indexing-service/src/main/java/org/apache/druid/indexing/worker/shuffle/LocalIntermediaryDataManager.java | Renames unused try-with-resources handle to ignored*. |
| indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java | Removes dead local variables. |
| indexing-service/src/main/java/org/apache/druid/indexing/overlord/sampler/InputSourceSampler.java | Renames unused try-with-resources handle to ignored*. |
| extensions-core/google-extensions/src/main/java/org/apache/druid/storage/google/GoogleTaskLogs.java | Removes unused parameter from task-file streaming helper and updates callers. |
| extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/RabbitStreamRecordSupplier.java | Refactors buffer-reset helper; currently introduces incorrect queue filtering during seek operations. |
| extensions-contrib/opentelemetry-emitter/src/main/java/org/apache/druid/emitter/opentelemetry/OpenTelemetryEmitter.java | Renames unused try-with-resources handle to ignored*. |
| codestyle/pmd-ruleset.xml | Adds PMD unused-parameter and unused-local-variable rules. |
Comments suppressed due to low confidence (4)
extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/RabbitStreamRecordSupplier.java:277
filterBufferAndResetBackgroundFetchnow filters the buffer using!streamBuilders.containsKey(...), which effectively drops buffered records for all assigned partitions (and thenhandle()has already advancedoffsetMapwhen offering). This can lead to message loss when seeking, since unpolled records for non-seeked partitions are discarded while their offsets have moved forward.
Pass the set of partitions being seeked into this method and only drop buffered records for those partitions.
This issue also appears in the following locations of the same file:
- line 288
- line 295
- line 304
private void filterBufferAndResetBackgroundFetch()
{
this.stopBackgroundFetch();
// filter records in buffer and only retain ones whose partition was not seeked
BlockingQueue<OrderedPartitionableRecord<String, Long, ByteEntity>> newQ = new LinkedBlockingQueue<>(
extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/RabbitStreamRecordSupplier.java:292
- After changing
filterBufferAndResetBackgroundFetchto take the seeked partitions,seek(...)should pass the partition being seeked so only its buffered records are discarded.
public void seek(StreamPartition<String> partition, Long sequenceNumber)
{
filterBufferAndResetBackgroundFetch();
offsetMap.put(partition.getPartitionId(), OffsetSpecification.offset(sequenceNumber));
}
extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/RabbitStreamRecordSupplier.java:301
- After changing
filterBufferAndResetBackgroundFetchto take the seeked partitions,seekToEarliest(...)should pass the providedpartitionsset so only those partitions’ buffered records are discarded.
public void seekToEarliest(Set<StreamPartition<String>> partitions)
{
filterBufferAndResetBackgroundFetch();
for (StreamPartition<String> part : partitions) {
offsetMap.put(part.getPartitionId(), OffsetSpecification.first());
}
}
extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/RabbitStreamRecordSupplier.java:309
- After changing
filterBufferAndResetBackgroundFetchto take the seeked partitions,seekToLatest(...)should pass the providedpartitionsset so only those partitions’ buffered records are discarded.
public void seekToLatest(Set<StreamPartition<String>> partitions)
{
filterBufferAndResetBackgroundFetch();
for (StreamPartition<String> part : partitions) {
offsetMap.put(part.getPartitionId(), OffsetSpecification.last());
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed the suppressed Copilot review warning in commit 9dcb608. RabbitStreamRecordSupplier now removes buffered records only for the partitions being seeked, preserving records from other assigned partitions. Added testSeekRetainsBufferedRecordsForOtherPartitions. The full RabbitStreamRecordSupplierTest class passes (9 tests), and Maven validate passes with Checkstyle, PMD, and Enforcer. |
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.
Reviewed 18 of 18 changed files.
This is an automated review by Codex GPT-5.6-Sol
Created by the GPT-5.6-Sol model.
Summary
UnusedFormalParameterandUnusedLocalVariablerules in the Maven static-check lifecycleignored*conventionThe broader CodeQL pattern also includes generated sources, public/interface parameters, and test scaffolding. This change focuses the Maven gate on actionable handwritten production code without breaking compatibility APIs or try-with-resources cleanup semantics.
Verification
mvn -ntp -B pmd:check -Dweb.console.skip=true -DskipTests -T1Cmvn -ntp -B test-compile -pl processing,server,indexing-service,sql,extensions-core/google-extensions,extensions-contrib/opentelemetry-emitter,extensions-contrib/rabbit-stream-indexing-service -am -Dweb.console.skip=true -DskipTests -T1C