Skip to content

build: detect unused parameters and local variables - #19821

Open
FrankChen021 wants to merge 2 commits into
apache:masterfrom
FrankChen021:agent/remove-unused-code
Open

build: detect unused parameters and local variables#19821
FrankChen021 wants to merge 2 commits into
apache:masterfrom
FrankChen021:agent/remove-unused-code

Conversation

@FrankChen021

Copy link
Copy Markdown
Member

Created by the GPT-5.6-Sol model.

Summary

  • enable PMD's UnusedFormalParameter and UnusedLocalVariable rules in the Maven static-check lifecycle
  • remove six unused parameters from private production methods and their call sites
  • remove two genuinely dead local variables
  • rename ten intentionally unused try-with-resources handles with PMD's supported ignored* convention

The 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 -T1C
  • mvn -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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 UnusedFormalParameter and UnusedLocalVariable rules in codestyle/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

  • filterBufferAndResetBackgroundFetch now filters the buffer using !streamBuilders.containsKey(...), which effectively drops buffered records for all assigned partitions (and then handle() has already advanced offsetMap when 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 filterBufferAndResetBackgroundFetch to 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 filterBufferAndResetBackgroundFetch to take the seeked partitions, seekToEarliest(...) should pass the provided partitions set 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 filterBufferAndResetBackgroundFetch to take the seeked partitions, seekToLatest(...) should pass the provided partitions set 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.

@FrankChen021

Copy link
Copy Markdown
Member Author

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 FrankChen021 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants