sarama icon indicating copy to clipboard operation
sarama copied to clipboard

fix: return ErrMessageSizeTooLarge when message is too large

Open ae-govau opened this issue 10 months ago • 5 comments

For most of this library's existence it has returned ErrMessageSizeTooLarge when the message exceeded the configured size.

For a short period in 2019 this error was renamed (#1218) but shortly revered back (#1262). Later in 2023 this error was changed to a ConfigurationError (#2628) to fix #2137, however this has caused issues with clients who rely on the previous error code being distinct from other ConfigurationError conditions (#2655).

This commit reverts to previous behaviour, and adds a test to pickup if this changes again in the future.

ae-govau avatar Apr 03 '24 23:04 ae-govau

@puellanivis - this might be a better approach? I know you like the errors.Is() but I added the equality check for the err in the tests as I suspect that's what most code-bases currently do.

@shermanCRL , @dnwe and @hindessm who reported #2137 and made #2628 which this effectively reverts who may be interested in this discussion and have comments.

An altnernate PR which doesn't change the current types returns is proposed in #2848. Personally I prefer this approach in this PR (ie restore previous behaviour), but that unresolves #2137 which may or may not be OK.

ae-govau avatar Apr 03 '24 23:04 ae-govau

I mean, one of the first tests errors.Is does is check to make sure the errors are comparable and then err == target. So, testing with equality here isn’t so bad (and is a stronger assertion for the package API tests, that we’re returning directly the ErrMessageSizeTooLarge and not any wrapped version.

puellanivis avatar Apr 04 '24 12:04 puellanivis

To be more clear, I’m more behind this revert, rather than adding a new function to test an error string “safely”.

puellanivis avatar Apr 05 '24 17:04 puellanivis

Pushed additional commit to fix the lint. Force-push because I forgot the DCO the first time.

ae-govau avatar Apr 09 '24 00:04 ae-govau

It's not clear to me why that one test failed in one env. Any chance that's a flaky test?

ae-govau avatar Apr 11 '24 23:04 ae-govau

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[bot] avatar Jul 11 '24 00:07 github-actions[bot]