You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "frankvicky (via GitHub)" <gi...@apache.org> on 2024/08/14 08:37:29 UTC
[PR] KAFKA-17324: Set config group.protocol to classic in Streams [kafka]
frankvicky opened a new pull request, #16878:
URL: https://github.com/apache/kafka/pull/16878
JIRA: [KAFKA-17324](https://issues.apache.org/jira/browse/KAFKA-17324)
> Streams is not compatible with the new consumer rebalance protocol proposed in KIP-848. Thus, Streams should set/override config `group.protocol` to `classic` at startup to ensure that the classic protocol is used.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17324: Set config group.protocol to classic in Streams [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on PR #16878:
URL: https://github.com/apache/kafka/pull/16878#issuecomment-2310037023
LGTM, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17324: Set config group.protocol to classic in Streams [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #16878:
URL: https://github.com/apache/kafka/pull/16878#discussion_r1721697187
##########
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java:
##########
@@ -470,6 +470,22 @@ public void shouldResetToDefaultIfRestoreConsumerAutoCommitIsOverridden() {
assertEquals("false", consumerConfigs.get(ConsumerConfig.ENABLE_AUTO_COMMIT_CONFIG));
}
+ @Test
+ public void shouldResetToDefaultIfConsumerGroupProtocolIsOverridden() {
+ props.put(StreamsConfig.consumerPrefix(ConsumerConfig.GROUP_PROTOCOL_CONFIG), "classic,consumer");
Review Comment:
Hello @lucasbru
Thank you for pointing that out. After double-checking the documentation, I realized that you’re absolutely right—"classic,consumer" is not a valid value in a common consumer config. I appreciate your attention to detail.
I’ll update the test case. Thanks again for your valuable feedback! 🙇🏼
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17324: Set config group.protocol to classic in Streams [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #16878:
URL: https://github.com/apache/kafka/pull/16878#discussion_r1721297070
##########
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java:
##########
@@ -470,6 +470,22 @@ public void shouldResetToDefaultIfRestoreConsumerAutoCommitIsOverridden() {
assertEquals("false", consumerConfigs.get(ConsumerConfig.ENABLE_AUTO_COMMIT_CONFIG));
}
+ @Test
+ public void shouldResetToDefaultIfConsumerGroupProtocolIsOverridden() {
+ props.put(StreamsConfig.consumerPrefix(ConsumerConfig.GROUP_PROTOCOL_CONFIG), "classic,consumer");
Review Comment:
nit: "classic,consumer" is not a valid value for the config is it? THat's a bit confusing, maybe I would just use "consumer"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17324: Set config group.protocol to classic in Streams [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru commented on code in PR #16878:
URL: https://github.com/apache/kafka/pull/16878#discussion_r1721688446
##########
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java:
##########
@@ -470,6 +470,22 @@ public void shouldResetToDefaultIfRestoreConsumerAutoCommitIsOverridden() {
assertEquals("false", consumerConfigs.get(ConsumerConfig.ENABLE_AUTO_COMMIT_CONFIG));
}
+ @Test
+ public void shouldResetToDefaultIfConsumerGroupProtocolIsOverridden() {
+ props.put(StreamsConfig.consumerPrefix(ConsumerConfig.GROUP_PROTOCOL_CONFIG), "classic,consumer");
Review Comment:
You are saying that "classic,consumer" is a valid value in a common consumer config, but I think it is not. That's why I would not put it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17324: Set config group.protocol to classic in Streams [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on code in PR #16878:
URL: https://github.com/apache/kafka/pull/16878#discussion_r1721311414
##########
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java:
##########
@@ -470,6 +470,22 @@ public void shouldResetToDefaultIfRestoreConsumerAutoCommitIsOverridden() {
assertEquals("false", consumerConfigs.get(ConsumerConfig.ENABLE_AUTO_COMMIT_CONFIG));
}
+ @Test
+ public void shouldResetToDefaultIfConsumerGroupProtocolIsOverridden() {
+ props.put(StreamsConfig.consumerPrefix(ConsumerConfig.GROUP_PROTOCOL_CONFIG), "classic,consumer");
Review Comment:
Hello @lucasbru,
Thanks for the review 🙇🏼.
In this test case, we aim to ensure that the group protocol is not overridden. The value "classic,consumer" is used here to verify that even if the user provides a valid protocol (valid in a common consumer config, but not in Kafka Streams), it will still be reset to "classic".
If this might cause confusion, I think adding a comment to clarify would be a good idea.
WYDT ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17324: Set config group.protocol to classic in Streams [kafka]
Posted by "lucasbru (via GitHub)" <gi...@apache.org>.
lucasbru merged PR #16878:
URL: https://github.com/apache/kafka/pull/16878
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org