-
Notifications
You must be signed in to change notification settings - Fork 14.8k
KAFKA-19793 Adding allow topic creation false for global and restore consumer #20723
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
Nikita-Shupletsov
left a comment
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.
thank you for the PR.
LGTM. @mjsax Could you please take a look?
|
@mjsax Can you review the PR ? |
| baseConsumerProps.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "none"); | ||
|
|
||
| // disable auto topic creation | ||
| baseConsumerProps.put(ConsumerConfig.ALLOW_AUTO_CREATE_TOPICS_CONFIG, "false"); |
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.
Why don't we add this to NON_CONFIGURABLE_CONSUMER_DEFAULT_CONFIGS and also set it centrally in getCommonConsumerConfigs() ?
For this case, we can also remove the corresponding line in getMainConsumerConfigs()
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.
@mjsax Done the changes.
Nikita-Shupletsov
left a comment
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
Hi @mjsax Can you also review and confirm ? |
streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java
Outdated
Show resolved
Hide resolved
streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java
Outdated
Show resolved
Hide resolved
streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java
Outdated
Show resolved
Hide resolved
…ectedUserSpecifiedClientConfig
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java
Outdated
Show resolved
Hide resolved
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| public void shouldLogErrorWhenUserTriesToOverrideAutoCreateTopics() { |
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.
Should we also duplicate this test, using the three individual consumer prefix? (or just merge this test into the one from above?
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 have merge the test @mjsax Please review.
|
Thanks for the fix. Merged to |
…consumer (apache#20723) Added auto topic creation false for both restore and global consumers. Reviewers: Nikita Shupletsov <nshupletsov@confluent.io>, Matthias J. Sax <matthias@confluent.io>
Added auto topic creation false for both restore and global consumers.
Reviewers: Nikita Shupletsov nshupletsov@confluent.io, Matthias J. Sax
matthias@confluent.io