Skip to content

KAFKA-20736: Improve removal of unassigned partitions from share sessions#22720

Open
AndrewJSchofield wants to merge 2 commits into
apache:trunkfrom
AndrewJSchofield:KAFKA-20736-1
Open

KAFKA-20736: Improve removal of unassigned partitions from share sessions#22720
AndrewJSchofield wants to merge 2 commits into
apache:trunkfrom
AndrewJSchofield:KAFKA-20736-1

Conversation

@AndrewJSchofield

@AndrewJSchofield AndrewJSchofield commented Jul 1, 2026

Copy link
Copy Markdown
Member

When all partitions in a share session are unassigned with no pending
acknowledgements to be sent, no ShareFetch request was being sent. This
patch sends a ShareFetch to remove the partitions from the share session
in this situation.

Reviewers: Shivsundar R shr@confluent.io, Apoorv Mittal
apoorvmittal10@gmail.com

if (nodesWithPendingRequests.contains(node.id())) {
log.trace("Skipping fetch because previous fetch request to {} has not been processed", nodeId);
} else {
Set<TopicPartition> currentPartitionsToFetch = new HashSet<>(partitionsToFetch());

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.

Is it possible to maybe include this logic the previous loop (the one processing the unsent acknowledgements) just after we do the acknowledgements check to avoid passing over the main map twice?
Since we are having the !handlerMap.containsKey(node) check here, it should not coincide with any partitions added in that acknowledgements loop pass.

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 thought about it, but the map is going to be small. I preferred having the logic separate of the third loop.

@ShivsundarR ShivsundarR 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.

Thanks @AndrewJSchofield, looks good overall. Just a minor comment.

@apoorvmittal10 apoorvmittal10 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.

Thanks for the PR, LGTM!

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

Labels

clients consumer KIP-932 Queues for Kafka small Small PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants