regression: unable to handle detect errors related to messages being too large

[ae-govau]

(creating issue from comments I made on #2628 a number weeks ago)

The recent change to error handling in #2628 broke some of our running code. We had code that would:

if err == sarama.ErrMessageSizeTooLarge {
    // log a message, bump a metric, but don't retry
}

ie for our use-case, we want to ignore messages related to a message being too large. It's OK for us to throw these out. We used to check a size before sending into Sarama, but then we had some cases where our payload was under the limit, but once headers etc were added, it was over, so we switched to checking this error code.

Noting that #543 reports that previous error code has been returned for at least 8 years, this seems a reasonably significant change... (and a good example of Hyrum's Law )

It's now not clear to me how I should check for size related issues.

Interestingly the last PR which touched that line of code was 4 years ago in 2019 where #1262 reverts a change a few days earlier (#1218) that renamed a bunch of error variables because:

renaming ErrXxx constants is an API changing change #1261

(that change is arguably less impactful than this one because it could be detected/corrected at compile time whereas we only caught this at runtime)

[ae-govau]

We're open to changing our code, but as a result of this change we've had to rewrite it as

if err == sarama.ErrMessageSizeTooLarge || strings.Contains(err.Error(), "MaxMessageBytes") {

and I feel like there should be a better option?

[dnwe]

@ae-govau would this be sufficient?

if errors.Is(err, sarama.ConfigurationError) ||
       errors.Is(err, sarama.ErrMessageSizeTooLarge) {
        // log non-retriable error and don’t retry send
}

[hindessm]

Apologies that my assumption that code would already be handling the configuration error was incorrect. Hopefully @dnwe's suggestion above helps.

@dnwe Perhaps the API should export a generic function to check for retriable/non-retriable errors for cases like this?

[ae-govau]

Thanks for the responses. We aren't that keen on checking for a generic configuration error, as we would like to be alerted to any other config errors - we just want to know when the problem is that the message is too big - since we deal with that failure mode quite differently than others.

Noting that the calculated size of the message (as I understand it) is bigger than the payload itself, it's not clear how we can calculate that before trying to send it and thus we need to be able to figure it out from the error itself.

An API for "is this a message too big error" would also be fine.

[shou1dwe]

Ran into the exact problem, and I am really reluctant to put in a string-based error type check like errors.Is(err, sarama.ConfigurationError) && strings.HasPrefix(string(err), "Attempt to produce message larger than configured Producer.MaxMessageBytes") for example.

I am afraid someone will make a "error message readability improvement" or a casing change that is seemingly harmless and cause a problem for us.

I appreciate the need to distinguish a client side error from broker error. But reusing ConfigurationError here doesn't sound right either. Arguably this isn't a ConfigurationError, because we configure the threshold correctly, and message produced can be sourced from other places so the size of it is a runtime behavior that is not entirely in the control of the system hosting producer. It is some flavor of "Bad Request" or "Invalid Argument" imho.

What makes it worse is that it is not a behavior in the mock producer, so I can't just add a unit test to detect the change in behavior here but have to use a real kafka client setup to detect it.

In any case, it would be nice to be some error value that we can reference in the code, or similar to what @ae-govau suggested, an API for "is this a message too big error".

[github-actions]

Thank you for taking the time to raise this issue. However, it has not had any activity on it in the past 90 days and will be closed in 30 days if no updates occur.
Please check if the main branch has already resolved the issue since it was raised. If you believe the issue is still valid and you would like input from the maintainers then please comment to ask for it to be reviewed.

[ae-govau]

This is still an issue for us. I might make a PR to try to address.

[ae-govau]

PR #2848 is opened so that clients can call:

// IsMessageSizeTooLarge returns true if the error relates the message size
// being either too large as reported by the broker, or too large because it
// exceeds the configured maximum size.
func IsMessageSizeTooLarge(err error) bool {
...

[Manicben]

Just adding how this has impacted us.

In our applications, the ErrMessageSizeTooLarge error was used to determine retriability. If it was returned by the brokers, we had logic to split up the batch provided to SendMessages to allow messages to be accepted by the brokers. If it was returned by the producer itself, or by the brokers after splitting down to a single message, then it was deemed non-retriable and the message would be handled as such (usually via DLQ).

In this case, checking for any ConfigurationError is not acceptable, and we'd have to inspect the error string to ensure it is due to exceeding Producer.MaxMessageBytes and nothing else. This means the suggestion from @dnwe doesn't work for us and as @ae-govau said, we cannot determine the message/batch size ourselves before calling SendMessage(s).

For now we will have to settle with checking the error string, but we would also be keen to see either a dedicated error type or some API like @ae-govau has suggested.

[github-actions]

Thank you for taking the time to raise this issue. However, it has not had any activity on it in the past 90 days and will be closed in 30 days if no updates occur.
Please check if the main branch has already resolved the issue since it was raised. If you believe the issue is still valid and you would like input from the maintainers then please comment to ask for it to be reviewed.