-
Notifications
You must be signed in to change notification settings - Fork 14.4k
KAFKA-18651: Add Streams-specific broker configurations #19176
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @aliehsaeedii ! I left some comments.
Can you please fix the PR title and make the PR description more precise? E.g. we want to mention that these are only the broker-side configs, and only configs required for features that we plan to implement in 4.1. This will form the commit message and will be the only permanent record of your intentions with this change (slack messages / tickets are not accessible by other project members), and otherwise the committer has to do these things when merging the PR.
@@ -59,6 +59,18 @@ public final class GroupConfig extends AbstractConfig { | |||
"Negative duration is not allowed.</li>" + | |||
"<li>anything else: throw exception to the share consumer.</li></ul>"; | |||
|
|||
public static final String STREAMS_SESSION_TIMEOUT_MS_CONFIG = "group.streams.session.timeout.ms"; | |||
public static final int STREAMS_SESSION_TIMEOUT_MS_DEFAULT = 45000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the defaults and docs in this file, I would just reference the GroupCoordinatorConfig
variants instead of creating a duplicate here, like the other group types do.
|
||
public static final String STREAMS_GROUP_MIN_SESSION_TIMEOUT_MS_CONFIG = "group.streams.min.session.timeout.ms"; | ||
public static final int STREAMS_GROUP_MIN_SESSION_TIMEOUT_MS_DEFAULT = 45000; | ||
public static final String STREAMS_GROUP_MIN_SESSION_TIMEOUT_MS_DOC = "The minimum session timeout."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it wouldn't be clearer if we refer to specific configurations here:
"The minimum for the group-level configuration of " + GroupConfig.STREAMS_GROUP_SESSION_TIMEOUT_MS_CONFIG
Same for all the other min/max configs below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@aliehsaeedii there seem to be test fialures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @aliehsaeedii ! I left some comments.
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorConfig.java
Outdated
Show resolved
Hide resolved
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorConfig.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unapproving
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @aliehsaeedii !
This PR implements the broker-side configs proposed in KIP-1071
The configurations implemented by this PR are only those that were specifically aimed to be included in
AK 4.1
.Reviewers: Lucas Brutschy [email protected]