Skip to content

MINOR: Fix potential NPE #21846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chia7712 merged 1 commit into from
Mar 26, 2026
+4 −1
Merged

MINOR: Fix potential NPE #21846

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion clients/src/main/java/org/apache/kafka/clients/consumer/internals/ShareFetch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,10 @@ public Map<TopicIdPartition, NodeAcknowledgements> takeAcknowledgedRecords() {
public int renew(Map<TopicIdPartition, Acknowledgements> acknowledgementsMap, Optional<Integer> acquisitionLockTimeoutMs) {
int recordsRenewed = 0;
for (Map.Entry<TopicIdPartition, Acknowledgements> entry : acknowledgementsMap.entrySet()) {
recordsRenewed += batches.get(entry.getKey()).renew(entry.getValue());
ShareInFlightBatch<K, V> batch = batches.get(entry.getKey());
Copy link
Copy Markdown
Member

@chia7712 chia7712 Mar 21, 2026

Choose a reason for hiding this comment

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

Good defensive check, but just out of curiosity, what causes this to end up being null?

if (batch != null) {
Copy link
Copy Markdown
Contributor

@nileshkumar3 nileshkumar3 Mar 21, 2026

Choose a reason for hiding this comment

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

Nice catch on the NPE. Only nit: can we add a brief note on renew that missing batches are intentionally skipped?

recordsRenewed += batch.renew(entry.getValue());
}
Copy link
Copy Markdown
Contributor

@aliehsaeedii aliehsaeedii Mar 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

1- Does that make sense to add such a test (as you mentioned you hitted in once during testing)
2- When a batch is not found, the code silently skips it. This might be correct behavior (I'm not sure), but
does that make sense to log sth there?

Copy link
Copy Markdown
Member Author

@AndrewJSchofield AndrewJSchofield Mar 25, 2026

Choose a reason for hiding this comment

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

My suspicion is that I temporarily broke the code while working on another PR on trunk and it revealed that this piece of code was a bit less defensive than it should have been. I'll finish off the larger trunk PR later this week, and I will enhance this small 4.2 PR if necessary. @chia7712 @nileshkumar3

}
acquisitionLockTimeoutMsRenewed = acquisitionLockTimeoutMs;
return recordsRenewed;
Expand Down
Loading