-
Notifications
You must be signed in to change notification settings - Fork 14.6k
KAFKA-19574: Improve producer and consumer config files #20302
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
This is an attempt at improving the client configuration files. We now have sections and comments similar to the other properties files. Signed-off-by: Federico Valeri <[email protected]>
@showuon fyi. Let me know if you want to add or remove anything. |
A label of 'needs-attention' was automatically added to this PR in order to raise the |
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 @fvaleri!
I like the idea of providing more of the configuration and some explanation behind it.
How do we keep this from getting out of sync with ConsumerConfig
and ProducerConfig
?
Hi @kirktrue, thanks for having a look. I added a couple of tests to ensure that these files remain in sync with config definitions. Let me know what you think. |
Signed-off-by: Federico Valeri <[email protected]>
Signed-off-by: Federico Valeri <[email protected]>
Perfect! Thanks! |
We need someone to add the |
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 improvement. This is definitely better than before.
clients/src/test/java/org/apache/kafka/clients/consumer/ConsumerConfigTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Federico Valeri <[email protected]>
@showuon @kirktrue we have 2 unrelated quarantined tests, but the rest looks good. Do you have additional comments on this? PlaintextConsumerTest > testAsyncConsumerCloseLeavesGroupOnInterrupt [1] Type=Raft-Isolated, MetadataVersion=4.2-IV1,BrokerSecurityProtocol=PLAINTEXT,BrokerListenerName=ListenerName(EXTERNAL),ControllerSecurityProtocol=PLAINTEXT,ControllerListenerName=ListenerName(CONTROLLER)
PlaintextConsumerTest > testClassicConsumerCloseLeavesGroupOnInterrupt [1] Type=Raft-Isolated, MetadataVersion=4.2-IV1,BrokerSecurityProtocol=PLAINTEXT,BrokerListenerName=ListenerName(EXTERNAL),ControllerSecurityProtocol=PLAINTEXT,ControllerListenerName=ListenerName(CONTROLLER) |
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!
@kirktrue , I'm going to merge this PR tomorrow if you don't have any other comments. Thanks. |
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 @fvaleri!
This is an attempt at improving the client configuration files. We now have sections and comments similar to the other properties files. Reviewers: Kirk True <[email protected]>, Luke Chen <[email protected]> --------- Signed-off-by: Federico Valeri <[email protected]>
This is an attempt at improving the client configuration files. We now have sections and comments similar to the other properties files.
Reviewers: Kirk True [email protected], Luke Chen [email protected]