fix: provide IsMessageSizeTooLarge() function#2848
Conversation
ghost
mentioned this pull request
Signed-off-by: Adam Eijdenberg <adam.eijdenberg@defence.gov.au>
| return true | ||
| } | ||
|
|
||
| return strings.Contains(err.Error(), messageSizeTooLargePrefix) |
There was a problem hiding this comment.
This really shouldn’t be something that we bake in. Checking the string value of the error is considered The Wrong Way™ to check error conditions. And once we provide this, the API is kind of stuck with it no matter what.
I think returning this as a ConfigurationError is itself faulty. We should be returning a type that allows errors.Is(err, ErrMessageSizeTooLarge) to cover all the functionality necessary.
There was a problem hiding this comment.
I'd argue encapsulating the string check as a private implementation detail of the library does the opposite of baking it in. e.g. currently users need to bake-in this logic (we have, as have others). Moving it to a function means that users won't need to update their logic each time this library chooses the change the error type that is returned (or changes the text in it).
The precise error returned for this condition has changed at least 3 times over the years so far:
- fix: make clear that error is configuration issue not server error #2628
- Revert "Synced error names and descriptions with the kafka's protocol" #1262
- Synced error names and descriptions with the kafka's protocol #1218
I agree it's not great, but I'm not sure that there is an alterative to this change that doesn't change the error type returned for a 4th time. (Although I'd be supportive of a simple revert of #2628 which would return ErrMessageSizeTooLarge again).
There was a problem hiding this comment.
I mean, it’s better than depending on raw strings copied into all our codebases, for sure. But I worry that it’s too much like putting wallpaper over a hole in the wall.
There will be a non-zero number of codebases that aren’t going to change their code to use this function, and just keep using the raw string, because “hey, if it ain’t broke, don’t fix it.” (Also, finding priority for maintenance work is almost always a struggle, since there’s a fairly ephemeral value to product.) And these people are still going to find themselves stuck out if/when the error text changes, and the breaking behavior change for these people will again be relatively silent until it causes the same critical/issue/alert/whatever that brought us all here.
🤷♀️ I’m just not sure… like I said, I don’t know if this improves the situation in any practical way. And in a year or two, someone will be looking at this code and wondering why we would ever double down on something much of Go considers an anti-pattern.
There was a problem hiding this comment.
I mean, I kind of hope no one removed the ErrMessageSizeTooLarge handling right? 😰 Because this was still a value that the server could be returning, and one would likely want to handle the same as if the client refused to even send the message.
ghost
mentioned this pull request
|
Closing in favour of #2851 |
ghost
closed this
1 participant
This provides a method for producers to workaround the regression introduced by #2628. Fixes #2655.