Skip to content

KAFKA-19156: Streamlined share group configs, with usage in ShareSessionCache #19505

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

Jump to bottom
Merged
merged 6 commits into from
Apr 23, 2025
Merged

KAFKA-19156: Streamlined share group configs, with usage in ShareSessionCache #19505

merged 6 commits into from
Apr 23, 2025

Conversation

chirag-wadhwa5
Copy link
Contributor

@chirag-wadhwa5 chirag-wadhwa5 commented Apr 17, 2025 •
edited by github-actions bot
Loading

This PR removes the group.share.max.groups config. This config was used
to calculate the maximum size of share session cache. But with the new
config group.share.max.share.sessions in place with exactly this
purpose, the ShareSessionCache initialization has also been passed the
new config.

Refer: KAFKA-19156

@github-actions github-actions bot added triage PRs from the community core Kafka Broker small Small PRs labels Apr 17, 2025
@AndrewJSchofield AndrewJSchofield added KIP-932 Queues for Kafka ci-approved and removed triage PRs from the community labels Apr 17, 2025
chirag-wadhwa5 referenced this pull request Apr 17, 2025
@chirag-wadhwa5
KAFKA-19157: added group.share.max.share.sessions config (#19503)
db62c7c 
This PR adds the config group.share.max.share.sessions to
ShareGroupConfig

Reviewers: Andrew Schofield <aschofield@confluent.io>
apoorvmittal10
apoorvmittal10 reviewed Apr 17, 2025
View reviewed changes
Copy link
Contributor

@apoorvmittal10 apoorvmittal10 left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor nit.

...rdinator/src/main/java/org/apache/kafka/coordinator/group/modern/share/ShareGroupConfig.java Outdated
@@ -104,7 +97,7 @@ public class ShareGroupConfig {
private final String shareGroupPersisterClassName;

public ShareGroupConfig(AbstractConfig config) {
// Share groups are enabled in two cases:
// Share groups are enabled in either of the two following cases:
// 1. The internal configuration to enable it is explicitly set
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// 1. The internal configuration to enable it is explicitly set
// 1. The internal configuration to enable it is explicitly set; or

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. Updated the comment

@chirag-wadhwa5 chirag-wadhwa5 mentioned this pull request Apr 17, 2025
Merged
chia7712
chia7712 reviewed Apr 17, 2025
View reviewed changes
...ator/src/test/java/org/apache/kafka/coordinator/group/modern/share/ShareGroupConfigTest.java
@@ -89,24 +87,6 @@ public void testInvalidConfigs() {
assertEquals("Invalid value 11 for configuration group.share.delivery.count.limit: Value must be no more than 10",
assertThrows(ConfigException.class, () -> createConfig(configs)).getMessage());

configs.clear();
// test for when SHARE_GROUP_MAX_GROUPS_CONFIG is of incorrect data type
configs.put(ShareGroupConfig.SHARE_GROUP_MAX_GROUPS_CONFIG, 10);
Copy link
Member

Choose a reason for hiding this comment

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

Should we add unit test for SHARE_GROUP_MAX_SHARE_SESSIONS_CONFIG?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is atleast(1) validation itself for the config. And KIP defines no maximum, so do we need any other validation?

Copy link
Member

Choose a reason for hiding this comment

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

There is atleast(1) validation itself for the config. And KIP defines no maximum, so do we need any other validation?

That is interesting. Should we increase the min value? Maybe it should be equal to or larger than group.share.max.size?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we increase the min value? Maybe it should be equal to or larger than group.share.max.size?

Yeah, that is something to consider. One should consider to have group.share.max.share.sessions >= group.share.max.size. @AndrewJSchofield wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

@apoorvmittal10 I think that makes a lot of sense.

apoorvmittal10
apoorvmittal10 reviewed Apr 23, 2025
View reviewed changes
Copy link
Contributor

@apoorvmittal10 apoorvmittal10 left a comment

Choose a reason for hiding this comment

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

The changes LGTM. 1 doubt I have here we validate that group.share.max.size should be minimum 1 but KIP-932 says as minimum to be 10, do we know the reason? And if we want group.share.max.size to be minimum as 10 then should we increase the minimum for group.share.max.share.sessions to 10 as well?

@AndrewJSchofield
Copy link
Member

The changes LGTM. 1 doubt I have here we validate that group.share.max.size should be minimum 1 but KIP-932 says as minimum to be 10, do we know the reason? And if we want group.share.max.size to be minimum as 10 then should we increase the minimum for group.share.max.share.sessions to 10 as well?

There's no right answer here for the minima. The defaults are more important, I think. The purpose of the minimum(1) is to prevent negative or zero values. If someone sets group.share.max.share.sessions to a value which is too low, consumption would be impacted, but setting the minimum value to 10 hardly makes any practical difference. So, I propose to leave the minimum values as they are. @apoorvmittal10 wdyt?

@apoorvmittal10
Copy link
Contributor

The changes LGTM. 1 doubt I have here we validate that group.share.max.size should be minimum 1 but KIP-932 says as minimum to be 10, do we know the reason? And if we want group.share.max.size to be minimum as 10 then should we increase the minimum for group.share.max.share.sessions to 10 as well?

There's no right answer here for the minima. The defaults are more important, I think. The purpose of the minimum(1) is to prevent negative or zero values. If someone sets group.share.max.share.sessions to a value which is too low, consumption would be impacted, but setting the minimum value to 10 hardly makes any practical difference. So, I propose to leave the minimum values as they are. @apoorvmittal10 wdyt?

Sounds good.

@apoorvmittal10 apoorvmittal10 changed the title KAFKA-19156: removed group.share.max.groups config and initialized ShareSessionCache with maxEntries set as group.share.max.share.sessions KAFKA-19156: Streamlined share group configs, with usage in ShareSessionCache Apr 23, 2025
@apoorvmittal10 apoorvmittal10 merged commit 4335027 into apache:trunk Apr 23, 2025
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chia7712 chia7712 chia7712 left review comments

@apoorvmittal10 apoorvmittal10 apoorvmittal10 approved these changes

@AndrewJSchofield AndrewJSchofield AndrewJSchofield approved these changes

Assignees
No one assigned
Labels
ci-approved core Kafka Broker KIP-932 Queues for Kafka small Small PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chirag-wadhwa5 @AndrewJSchofield @apoorvmittal10 @chia7712