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: return ErrMessageSizeTooLarge when message is too large #2851

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ae-govau
Copy link

@ae-govau ae-govau commented Apr 3, 2024

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.

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 (IBM#1218) but shortly
revered back (IBM#1262). Later in 2023 this error was changed to a
ConfigurationError (IBM#2628) to fix IBM#2137, however this has caused issues
with clients who rely on the previous error code being distinct from
other ConfigurationError conditions (IBM#2655).

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

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

ae-govau commented Apr 3, 2024

@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.

@puellanivis
Copy link
Contributor

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
Copy link
Contributor

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

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

ae-govau commented Apr 9, 2024

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

@ae-govau
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ErrMessageSizeTooLarge indicates a kafka server response when the error is in the Producer
2 participants