Skip to content
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

fix: allow skipping per message size check #2892

Closed
wants to merge 1 commit into from

Conversation

ae-govau
Copy link

@ae-govau ae-govau commented May 8, 2024

This is useful when producer compression is enabled, as it allows for much larger messages to be sent to the broker (providing the entire batch they are in stays small enough).

The previous workaround suggested in #2142 causes other ill-effects related to message batching (such as in #2797).

Fixes #2142, probably also #2797 and may help mitigate not having merged #2851.

It's not clear how easy it is to write a test for this but happy to try if we're happy enough with this approach.

This is useful when producer compression is enabled, as it allows for
much larger messages to be sent to the broker (providing the entire
batch they are in stays small enough).

Signed-off-by: Adam Eijdenberg <adam.eijdenberg@defence.gov.au>
@ae-govau
Copy link
Author

ae-govau commented May 8, 2024

This still doesn't solve everything -

sarama/produce_set.go

Lines 253 to 255 in 0ab2bb7

case ps.msgs[msg.Topic] != nil && ps.msgs[msg.Topic][msg.Partition] != nil &&
ps.msgs[msg.Topic][msg.Partition].bufferBytes+msg.ByteSize(version) >= ps.parent.conf.Producer.MaxMessageBytes:
return true
still is checking a before-compression amount for calculating the batch, but I think the above is probably better than erroneously returning errors for small messages when MaxMessageBytes is set too high.

@puellanivis
Copy link
Contributor

puellanivis commented May 8, 2024

The phrasing of “skip” here is potentially confusing, especially when batch processing. Not sure there’s a better phrasing possible though.

PS: Maybe Disable?

Copy link

github-actions bot commented Aug 6, 2024

Thank you for your contribution! However, this pull request has not had any activity in the past 90 days and will be closed in 30 days if no updates occur.
If you believe the changes are still valid then please verify your branch has no conflicts with main and rebase if needed. If you are awaiting a (re-)review then please let us know.

@github-actions github-actions bot added the stale Issues and pull requests without any recent activity label Aug 6, 2024
@github-actions github-actions bot closed this Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues and pull requests without any recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compressed size is not used for validating message in producer when compression is enabled
2 participants