Repository navigation
feat(producer): support per-topic message size limits - #3822
JamesMGreene wants to merge 3 commits into
Conversation
Add optional per-topic overrides for producer message and batch size checks while preserving the global fallback behavior. Signed-off-by: James M. Greene <JamesMGreene@github.com>
|
@dnwe Would greatly appreciate a review so I can at least judge if this might get merged, or if I need to address this separately within my own codebase instead. Thank you! 🙇🏻 |
Return only the resolved limit from producerMaxMessageBytes, check override presence only when selecting the validation error, and use sorted map keys for deterministic configuration validation. Add coverage for an explicit topic limit equal to the global limit. Signed-off-by: James M. Greene <JamesMGreene@github.com>
|
@puellanivis Thank you for the review! I've made some updates based on your feedback. Would greatly appreciate a re-review when you get a chance. 🙇🏻 |
puellanivis
left a comment
There was a problem hiding this comment.
Really just the one thing I noticed.
|
@puellanivis Before I make any more changes, I'm hitting a bit of a personal design crisis that could use your guidance. 🙏🏻 This PR is currently designed to allow the Topic-level overrides to be either smaller OR larger than the Producer-level config. This means it's treating the Producer-level config as more of a default/fallback limit rather than an absolute maximum limit. Does that seem like the most desirable approach for this library, or would it be better if the Topic-level overrides could only be smaller than ( Either way works fine for my particular use case (so far, anyway), so please advise what would be best for the library. Thank you! 🙇🏻 |
|
I don’t have any feedback or bias on which way we should go: either enforce “must be less than” or “can be anything”. We just want the warning message and the enforcement to align. I suppose one could look at the Java implementation(s)(?) to look at what the correct design choice here should be. But Personally, either works from a code quality perspective (which is where my code reviews usually focus). |
|
I didn't spot a specific implementation of this in the Java client, but all of the Kafka documentation (and their AI chatbot) seem to suggest that the topic-level configuration is indeed an override of the default, rather than only being allowed to reduce the size. From a session with their AI chatbot:
Supporting documentation:
|
Reject TopicMaxMessageBytes values at or above MaxRequestSize during validation instead of warning that they will be ignored without enforcing that behavior. Signed-off-by: James M. Greene <JamesMGreene@github.com>
TL;DR
Add optional per-topic overrides for producer message and batch size checks while preserving the global fallback behavior.
Producer.MaxMessageBytesoverrides #3697Problem
Sarama currently uses one global
Producer.MaxMessageBytesvalue for two jobs:(topic, partition)batch should roll overKafka's
max.message.bytescan differ by topic. When one producer writes to topics with different limits, the global setting either allows an oversized batch for a smaller topic or rejects a valid message for a larger one.This PR adds the opt-in per-topic override proposed in #3697.
What changed
Producer.TopicMaxMessageBytes map[string]int.Producer.MaxMessageByteswhen no override exists, preserving existing behavior by default.MaxRequestSize.This does not discover broker configuration or guarantee an exact post-compression batch size. The caller supplies the topic limits, and Sarama applies them to the same local size checks it already performs.
Testing
Note
AI assistance was used to help generate the new tests. I reviewed the test cases and verified that the full test suite passes.