You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "lucasbru (via GitHub)" <gi...@apache.org> on 2025/02/19 08:34:50 UTC
[PR] [WIP] MINOR: Forbid unused locals [kafka]
lucasbru opened a new pull request, #18963:
URL: https://github.com/apache/kafka/pull/18963
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on PR #18963:
URL: https://github.com/apache/kafka/pull/18963#issuecomment-2683609144
@lucasbru try updating this PR with trunk. That should fix the build scan errors
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1970132884
##########
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()) {
Review Comment:
This actually looks like a bug to me
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1970569660
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/MonitorableSinkConnector.java:
##########
@@ -60,7 +60,7 @@ public void start(Map<String, String> props) {
@Override
public void put(Collection<SinkRecord> records) {
super.put(records);
- for (SinkRecord ignore : records) {
+ for (@SuppressWarnings("UnusedLocalVariable") SinkRecord ignore : records) {
Review Comment:
Done
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1971693097
##########
tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java:
##########
@@ -79,7 +79,8 @@ public static void main(String[] args) {
shareConsumers.forEach(shareConsumer -> shareConsumersMetrics.add(shareConsumer.metrics()));
}
shareConsumers.forEach(shareConsumer -> {
- Map<TopicIdPartition, Optional<KafkaException>> val = shareConsumer.commitSync();
+ @SuppressWarnings("UnusedLocalVariable")
+ Map<TopicIdPartition, Optional<KafkaException>> ignored = shareConsumer.commitSync();
Review Comment:
I completely missed that this was in `tools`. I agree that the approach here (the explicit suppression) is better
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1971544479
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/DefaultStateUpdater.java:
##########
@@ -885,7 +885,8 @@ public Set<StreamTask> drainRestoredActiveTasks(final Duration timeout) {
restoredActiveTasksLock.lock();
try {
while (restoredActiveTasks.isEmpty() && now <= deadline) {
- final boolean elapsed = restoredActiveTasksCondition.await(deadline - now, TimeUnit.MILLISECONDS);
+ @SuppressWarnings("UnusedLocalVariable")
+ final boolean ignored = restoredActiveTasksCondition.await(deadline - now, TimeUnit.MILLISECONDS);
Review Comment:
I had to revert this. Spotbugs actually requires handling the return value here.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1976505917
##########
clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java:
##########
@@ -397,7 +397,7 @@ public void testStressfulSituation() throws Exception {
List<ProducerBatch> batches = accum.drain(metadataCache, nodes, 5 * 1024, 0).get(node1.id());
if (batches != null) {
for (ProducerBatch batch : batches) {
- for (Record record : batch.records().records())
+ for (@SuppressWarnings("UnusedLocalVariable") Record ignored : batch.records().records())
read++;
Review Comment:
Why not use `ProducerBatch#recordCount` and avoid the loop entirely?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -621,6 +621,7 @@ protected void assertInitializedMetric(WorkerConnector workerConnector, String e
String type = metrics.currentMetricValueAsString(metricGroup, "connector-type");
String clazz = metrics.currentMetricValueAsString(metricGroup, "connector-class");
String version = metrics.currentMetricValueAsString(metricGroup, "connector-version");
+ assertEquals("unassigned", status);
Review Comment:
Aha! Already found one test gap. Loving it.
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/DefaultStateUpdater.java:
##########
@@ -885,7 +885,8 @@ public Set<StreamTask> drainRestoredActiveTasks(final Duration timeout) {
restoredActiveTasksLock.lock();
try {
while (restoredActiveTasks.isEmpty() && now <= deadline) {
- final boolean elapsed = restoredActiveTasksCondition.await(deadline - now, TimeUnit.MILLISECONDS);
+ @SuppressWarnings("UnusedLocalVariable")
+ final boolean ignored = restoredActiveTasksCondition.await(deadline - now, TimeUnit.MILLISECONDS);
Review Comment:
Should we add a comment why it is ok to ignore the result?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1973200080
##########
tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java:
##########
@@ -79,7 +79,8 @@ public static void main(String[] args) {
shareConsumers.forEach(shareConsumer -> shareConsumersMetrics.add(shareConsumer.metrics()));
}
shareConsumers.forEach(shareConsumer -> {
- Map<TopicIdPartition, Optional<KafkaException>> val = shareConsumer.commitSync();
+ @SuppressWarnings("UnusedLocalVariable")
+ Map<TopicIdPartition, Optional<KafkaException>> ignored = shareConsumer.commitSync();
Review Comment:
This is OK with me in this case. It's just making sure that any in-flight acknowledgements are committed before the consumer is closed in a performance test.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1971542987
##########
tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java:
##########
@@ -79,7 +79,8 @@ public static void main(String[] args) {
shareConsumers.forEach(shareConsumer -> shareConsumersMetrics.add(shareConsumer.metrics()));
}
shareConsumers.forEach(shareConsumer -> {
- Map<TopicIdPartition, Optional<KafkaException>> val = shareConsumer.commitSync();
+ @SuppressWarnings("UnusedLocalVariable")
+ Map<TopicIdPartition, Optional<KafkaException>> ignored = shareConsumer.commitSync();
Review Comment:
I think the local was introduced because of a false alarm in spotbugs. It claims `commitSync` is side effect free (which is not true). We need to either annotate `commitSync` with `@CanIgnoreReturnValue` or have this workaround. I'd prefer having this workaround, as this is just performance testing code and we don't want to annotate user-facing interfaces like the share consumer with spotbugs annotations.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1970571792
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/DefaultStateUpdater.java:
##########
@@ -885,7 +885,8 @@ public Set<StreamTask> drainRestoredActiveTasks(final Duration timeout) {
restoredActiveTasksLock.lock();
try {
while (restoredActiveTasks.isEmpty() && now <= deadline) {
- final boolean elapsed = restoredActiveTasksCondition.await(deadline - now, TimeUnit.MILLISECONDS);
+ @SuppressWarnings("UnusedLocalVariable")
+ final boolean ignored = restoredActiveTasksCondition.await(deadline - now, TimeUnit.MILLISECONDS);
Review Comment:
It really does not matter in that case whether we got interrupted or the timeout expired. We could consider just removing the local here. It will be flagged by intellij though.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1970569977
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java:
##########
@@ -2881,7 +2881,7 @@ private void verifySessionPartitions() {
field.setAccessible(true);
LinkedHashMap<?, ?> sessionPartitions =
(LinkedHashMap<?, ?>) field.get(handler);
- for (Map.Entry<?, ?> entry : sessionPartitions.entrySet()) {
+ for (@SuppressWarnings("UnusedLocalVariable") Map.Entry<?, ?> ignored : sessionPartitions.entrySet()) {
Review Comment:
Done, also in some other place
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on PR #18963:
URL: https://github.com/apache/kafka/pull/18963#issuecomment-2707170074
@lucasbru yea, I think we gave enough time for any concerned parties to chime in :)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "cadonna (via GitHub)" <gi...@apache.org>.
cadonna commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1971354797
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/DefaultStateUpdater.java:
##########
@@ -885,7 +885,8 @@ public Set<StreamTask> drainRestoredActiveTasks(final Duration timeout) {
restoredActiveTasksLock.lock();
try {
while (restoredActiveTasks.isEmpty() && now <= deadline) {
- final boolean elapsed = restoredActiveTasksCondition.await(deadline - now, TimeUnit.MILLISECONDS);
+ @SuppressWarnings("UnusedLocalVariable")
+ final boolean ignored = restoredActiveTasksCondition.await(deadline - now, TimeUnit.MILLISECONDS);
Review Comment:
Yeah, @lucasbru got it right. Avoiding the flag by IntelliJ was the reason I added that variable.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1977199932
##########
clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java:
##########
@@ -397,7 +397,7 @@ public void testStressfulSituation() throws Exception {
List<ProducerBatch> batches = accum.drain(metadataCache, nodes, 5 * 1024, 0).get(node1.id());
if (batches != null) {
for (ProducerBatch batch : batches) {
- for (Record record : batch.records().records())
+ for (@SuppressWarnings("UnusedLocalVariable") Record ignored : batch.records().records())
read++;
Review Comment:
I think the point of the test is actually simulating consumption of the batch in a scenario where multiple threads append to the accumulator. So we want to actually access the `MemoryRecords`, not just test the record counter.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on PR #18963:
URL: https://github.com/apache/kafka/pull/18963#issuecomment-2706888847
@mumrah I have addressed all comments. Should we just go forward with this one?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru merged PR #18963:
URL: https://github.com/apache/kafka/pull/18963
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Disallow unused local variables [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #18963:
URL: https://github.com/apache/kafka/pull/18963#discussion_r1970287405
##########
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();
Review Comment:
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:
##########
@@ -885,7 +885,8 @@ public Set<StreamTask> drainRestoredActiveTasks(final Duration timeout) {
restoredActiveTasksLock.lock();
try {
while (restoredActiveTasks.isEmpty() && now <= deadline) {
- final boolean elapsed = restoredActiveTasksCondition.await(deadline - now, TimeUnit.MILLISECONDS);
+ @SuppressWarnings("UnusedLocalVariable")
+ final boolean ignored = restoredActiveTasksCondition.await(deadline - now, TimeUnit.MILLISECONDS);
Review Comment:
@cadonna do we need to check the result of the await here?
##########
tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java:
##########
@@ -79,7 +79,8 @@ public static void main(String[] args) {
shareConsumers.forEach(shareConsumer -> shareConsumersMetrics.add(shareConsumer.metrics()));
}
shareConsumers.forEach(shareConsumer -> {
- Map<TopicIdPartition, Optional<KafkaException>> val = shareConsumer.commitSync();
+ @SuppressWarnings("UnusedLocalVariable")
+ Map<TopicIdPartition, Optional<KafkaException>> ignored = shareConsumer.commitSync();
Review Comment:
@AndrewJSchofield do we need to check the result of commitSync?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/MonitorableSinkConnector.java:
##########
@@ -60,7 +60,7 @@ public void start(Map<String, String> props) {
@Override
public void put(Collection<SinkRecord> records) {
super.put(records);
- for (SinkRecord ignore : records) {
+ for (@SuppressWarnings("UnusedLocalVariable") SinkRecord ignore : records) {
Review Comment:
Since `records` is a Collection, can we just do `count += records.size()` ?
##########
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##########
@@ -960,6 +960,7 @@ public void copyLogSegmentsToRemote(UnifiedLog log) throws InterruptedException
// back to the caller. It's important to note that the task being executed is already
// cancelled before the executing thread is interrupted. The caller is responsible
// for handling the exception gracefully by checking if the task is already cancelled.
+ @SuppressWarnings("UnusedLocalVariable")
boolean ignored = copyQuotaManagerLockCondition.await(quotaTimeout().toMillis(), TimeUnit.MILLISECONDS);
Review Comment:
@abhijeetk88 @satishd, is it safe to ignore the result of the await here?
##########
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()) {
Review Comment:
@cmccabe or @mannoopj can you comment?
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java:
##########
@@ -2881,7 +2881,7 @@ private void verifySessionPartitions() {
field.setAccessible(true);
LinkedHashMap<?, ?> sessionPartitions =
(LinkedHashMap<?, ?>) field.get(handler);
- for (Map.Entry<?, ?> entry : sessionPartitions.entrySet()) {
+ for (@SuppressWarnings("UnusedLocalVariable") Map.Entry<?, ?> ignored : sessionPartitions.entrySet()) {
Review Comment:
I tried
```
sessionPartitions.forEach((key, value) -> Thread.yield());
```
and the checkstyle did not complain. It seems that the unused check does not care about lambda arguments.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org