Skip to content

KAFKA-19164: Keep track of groups when deleting transactional offsets #19495

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.

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

Conversation

squah-confluent
Copy link
Contributor

@squah-confluent squah-confluent commented Apr 16, 2025

When deleting pending transactional offsets, we must preserve the list
of groups associated with the producer ID, otherwise we cannot clean up
the list of pending transactions for the group once the transaction is
committed or aborted.

When deleting pending transactional offsets, we must preserve the list
of groups associated with the producer ID, otherwise we cannot clean up
the list of pending transactions for the group once the transaction is
committed or aborted.
@github-actions github-actions bot added triage PRs from the community small Small PRs labels Apr 16, 2025
Comment on lines +266 to 267
if (!preserveGroups && topicOffsets.isEmpty())
offsetsByGroup.remove(groupId);
Copy link
Contributor Author

@squah-confluent squah-confluent Apr 16, 2025

Choose a reason for hiding this comment

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

We use offsetsByGroup to clean up the list of open transactions per group.

pendingOffsets.offsetsByGroup.keySet().forEach(groupId -> {
TimelineHashSet<Long> openTransactions = openTransactionsByGroup.get(groupId);
if (openTransactions != null) {
openTransactions.remove(producerId);
if (openTransactions.isEmpty()) {
openTransactionsByGroup.remove(groupId);
}
}
});

If we do not maintain an entry here, the clean up will not happen. Later, we will not delete the group when we expire its last offset.

// We don't want to remove the group if there are ongoing transactions.
return allOffsetsExpired.get() && !openTransactionsByGroup.containsKey(groupId);

Copy link
Member

Choose a reason for hiding this comment

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

I guess we could also just remove from the openTransactionsByGroup here as well? Not a strong opinion either way. If we do it this way we should also clean it up when we remove from openTransactionByGroup? I don't think we want to preserve it forever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we could. It depends on whether we are trying to track whether a group has an open transaction with offset commits that will take effect when committed or whether a group has an open transaction at all.

We certainly shouldn't delete groups with uncommitted offsets, since they could be committed afterwards and would then not get cleaned up. I'm just not sure about groups with deleted uncommitted offsets that are a no-op when committed.

@dajac What should we be doing in the second case?

@github-actions github-actions bot removed the triage PRs from the community label Apr 17, 2025
@dajac dajac self-requested a review April 17, 2025 10:03
@dajac dajac added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label Apr 17, 2025
@squah-confluent
Copy link
Contributor Author

Closing this since #19497 fixes the bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-approved KIP-848 The Next Generation of the Consumer Rebalance Protocol small Small PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants