-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Vmhtuan/kafka producer config #26130
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
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Tuấn Vũ Minh.
|
ebyhr
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.
Please document new config properties in event-listeners-kafka.md, and squash commits into one.
| private Long maxRequestSize = 5242880L; | ||
| private Long batchSize = 16384L; |
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.
Could you leave a code comment explaining how these values are chosen?
Please use long instead.
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 reviewing! This PR has some issues, and I have closed it. I have opened a new PR at #26132. And I'm gonna resolve your comments. Thanks!
| return this; | ||
| } | ||
|
|
||
| public Long getMaxRequestSize() |
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.
getters/setters place should be consistent with fields.
| return maxRequestSize; | ||
| } | ||
|
|
||
| @Config("kafka-event-listener.max-request-size") |
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.
@ConfigDescription is missing. Same for kafka-event-listener.batch-size
| @Config("kafka-event-listener.max-request-size") | ||
| public KafkaEventListenerConfig setMaxRequestSize(Long maxRequestSize) | ||
| { | ||
| Optional.ofNullable(maxRequestSize).ifPresent(value -> this.maxRequestSize = value); |
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.
this.maxRequestSize = maxRequestSize;| @Config("kafka-event-listener.batch-size") | ||
| public KafkaEventListenerConfig setBatchSize(Long batchSize) | ||
| { | ||
| Optional.ofNullable(batchSize).ifPresent(value -> this.batchSize = value); |
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.
this.batchSize = batchSize;
Fixes #26129
Description
Add support for
max.request.sizeandbatch.sizeconfiguration for Kafka producer of Kafka Event ListenerAdditional context and related issues
Release notes
Release notes are required, with the following suggested text: