MINOR: Disallow unused local variables #18963

lucasbru · 2025-02-19T08:34:47Z

Recently, we found a regression that could have been detected by static analysis, since a local variable wasn't being passed to a method during a refactoring, and was left unused. It was fixed in 7a749b5, but almost slipped into 4.0. Unused variables are typically detected by IDEs, but this is insufficient to prevent these kinds of bugs. This change enables unused local variable detection in checkstyle for Kafka.

A few notes on the usage:

There are two situations in which people actually want to have a local variable but not use it. First, there are for (Type ignored: collection) loops which have to loop collection.length number of times, but that do not use ignored in the loop body. These are typically still easier to read than a classical for loop. Second, some IDEs detect it if a return value of a function such as File.delete is not being used. In this case, people sometimes store the result in an unused local variable to make ignoring the return value explicit and to avoid the squiggly lines.
In Java 22, unsued local variables can be omitted by using a single underscore _. This is supported by checkstyle. In pre-22 versions, IntelliJ allows such variables to be named ignored to suppress the unused local variable warning. This pattern is often (but not consistently) used in the Kafka codebase. This is, however, not supported by checkstyle.

Since we cannot switch to Java 22, yet, and we want to use automated detection using checkstyle, we have to resort to prefixing the unused local variables with @SuppressWarnings("UnusedLocalVariable"). We have to apply this in 11 cases across the Kafka codebase. While not being pretty, I'd argue it's worth it to prevent bugs like the one fixed in 7a749b5.

Reviewers: Andrew Schofield [email protected], David Arthur [email protected], Matthias J. Sax [email protected], Bruno Cadonna [email protected], Kirk True [email protected]

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

lucasbru · 2025-02-25T16:28:50Z

generator/src/main/java/org/apache/kafka/message/checker/EvolutionVerifier.java

@@ -74,7 +74,7 @@ static void verifyVersionsMatchTopLevelMessage(
            verifyVersionsMatchTopLevelMessage(what, topLevelMessage, field);
        }
        for (StructSpec struct : topLevelMessage.commonStructs()) {
-            for (FieldSpec field : topLevelMessage.fields()) {
+            for (FieldSpec field : struct.fields()) {


This actually looks like a bug to me

@cmccabe or @mannoopj can you comment?

mumrah

Thanks for the patch @lucasbru! I'm always in favor of safety improvements :)

I don't love that we have to suppress warnings, but I understand we're a bit constrained due to our wide range of Java versions. It doesn't look like we have too many required suppressions.

For a few places where you have created an ignored variable and a suppression, I noticed something interesting. If we remove the assignment, I get an IDEA warning about ignoring the result of a method. However, it does not fail the checkstyle.

I think we should evaluate each of these individually to determine if its safe to ignore the result. I left comments on these inline.

mumrah · 2025-02-25T17:57:11Z

core/src/main/java/kafka/log/remote/RemoteLogManager.java

+                                    @SuppressWarnings("UnusedLocalVariable")
                                    boolean ignored = copyQuotaManagerLockCondition.await(quotaTimeout().toMillis(), TimeUnit.MILLISECONDS);


@abhijeetk88 @satishd, is it safe to ignore the result of the await here?

mumrah · 2025-02-25T17:58:00Z

generator/src/main/java/org/apache/kafka/message/checker/EvolutionVerifier.java

@@ -74,7 +74,7 @@ static void verifyVersionsMatchTopLevelMessage(
            verifyVersionsMatchTopLevelMessage(what, topLevelMessage, field);
        }
        for (StructSpec struct : topLevelMessage.commonStructs()) {
-            for (FieldSpec field : topLevelMessage.fields()) {
+            for (FieldSpec field : struct.fields()) {


@cmccabe or @mannoopj can you comment?

mumrah · 2025-02-25T18:01:03Z

storage/src/main/java/org/apache/kafka/storage/internals/log/LogLoader.java

@@ -169,6 +169,7 @@ public LoadedLogOffsets load() throws IOException {
                    long offset = LogFileUtils.offsetFromFile(file);
                    if (offset >= minSwapFileOffset && offset < maxSwapFileOffset) {
                        logger.info("Deleting segment files {} that is compacted but has not been deleted yet.", file.getName());
+                        @SuppressWarnings("UnusedLocalVariable")
                        boolean ignore = file.delete();


Shouldn't we actually check if the file was deleted?

@mimaison I see you worked the Java conversion of this class. WDYT about checking the result of delete and renameTo below?

streams/src/main/java/org/apache/kafka/streams/processor/internals/DefaultStateUpdater.java

tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java

clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java

...ect/runtime/src/test/java/org/apache/kafka/connect/integration/MonitorableSinkConnector.java

kirktrue

Nice improvement @lucasbru!

I know that in some places in Java you can use __ to ignore a parameter, but this seems like a more comprehensive solution. I'm actually surprised there are so few places with unused variables.

mumrah · 2025-02-26T00:42:38Z

@lucasbru try updating this PR with trunk. That should fix the build scan errors

mjsax

Thanks a lot for this PR. Love it to see what we are tightening the checks to increase code quality.

Overall LGTM, besides the few open questions raise by others (also a few minor comments from my side).

It was fixed in 7a749b5, but almost slipped into 4.0.

Actually, this refactoring happend many releases back, and the bug was shipped with 3.4 release... We just found it by change, and fixed it for 4.0 release.

clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java

connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java

streams/src/main/java/org/apache/kafka/streams/processor/internals/DefaultStateUpdater.java

lucasbru · 2025-03-07T16:27:37Z

@mumrah I have addressed all comments. Should we just go forward with this one?

mumrah · 2025-03-07T18:45:04Z

@lucasbru yea, I think we gave enough time for any concerned parties to chime in :)

mumrah

LGTM

github-actions bot added streams core Kafka Broker producer consumer tools connect kraft mirror-maker-2 storage Pull requests that target the storage module tiered-storage Related to the Tiered Storage feature generator RPC and Record code generator clients labels Feb 19, 2025

lucasbru changed the title ~~[WIP] MINOR: Forbid unused locals~~ MINOR: Forbid unused locals Feb 25, 2025

lucasbru changed the title ~~MINOR: Forbid unused locals~~ MINOR: Disallow unused locals Feb 25, 2025

lucasbru changed the title ~~MINOR: Disallow unused locals~~ MINOR: Disallow unused local variables Feb 25, 2025

lucasbru commented Feb 25, 2025

View reviewed changes

mumrah reviewed Feb 25, 2025

View reviewed changes

kirktrue reviewed Feb 25, 2025

View reviewed changes

lucasbru force-pushed the disallow_locals branch from ae26e95 to 1f4e041 Compare February 26, 2025 10:36

github-actions bot added the KIP-932 Queues for Kafka label Feb 26, 2025

mjsax reviewed Mar 1, 2025

View reviewed changes

lucasbru added 6 commits March 3, 2025 12:08

impl

0598b78

comments / fewer suppressions

f9667fe

remove local in state updater

7a0614e

remove new unused local

66b58fb

spotbugs

6a401b2

explain why we ignore the return value from await

15f61c4

lucasbru force-pushed the disallow_locals branch from 4705420 to 15f61c4 Compare March 3, 2025 11:10

Merge remote-tracking branch 'apache/trunk' into disallow_locals

fbe74e9

mumrah approved these changes Mar 7, 2025

View reviewed changes

lucasbru merged commit fc2e3df into apache:trunk Mar 10, 2025
22 of 23 checks passed

mjsax mentioned this pull request Mar 11, 2025

MINOR: Removed unused local variable in method DefaultStateUpdater.drainRestoredActiveTasks #18385

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: Disallow unused local variables #18963

MINOR: Disallow unused local variables #18963

lucasbru commented Feb 19, 2025 •

edited

Loading

lucasbru Feb 25, 2025

mumrah Feb 25, 2025

mumrah left a comment

mumrah Feb 25, 2025

mumrah Feb 25, 2025

mumrah Feb 25, 2025

kirktrue left a comment

mumrah commented Feb 26, 2025

mjsax left a comment

lucasbru commented Mar 7, 2025

mumrah commented Mar 7, 2025

mumrah left a comment

		@SuppressWarnings("UnusedLocalVariable")
		boolean ignored = copyQuotaManagerLockCondition.await(quotaTimeout().toMillis(), TimeUnit.MILLISECONDS);

MINOR: Disallow unused local variables #18963

MINOR: Disallow unused local variables #18963

Conversation

lucasbru commented Feb 19, 2025 • edited Loading

Committer Checklist (excluded from commit message)

lucasbru Feb 25, 2025

Choose a reason for hiding this comment

mumrah Feb 25, 2025

Choose a reason for hiding this comment

mumrah left a comment

Choose a reason for hiding this comment

mumrah Feb 25, 2025

Choose a reason for hiding this comment

mumrah Feb 25, 2025

Choose a reason for hiding this comment

mumrah Feb 25, 2025

Choose a reason for hiding this comment

kirktrue left a comment

Choose a reason for hiding this comment

mumrah commented Feb 26, 2025

mjsax left a comment

Choose a reason for hiding this comment

lucasbru commented Mar 7, 2025

mumrah commented Mar 7, 2025

mumrah left a comment

Choose a reason for hiding this comment

lucasbru commented Feb 19, 2025 •

edited

Loading