KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

[kirktrue]

Jira: https://issues.apache.org/jira/browse/KAFKA-12841

Using the InterceptorCallback wrapper in the case of ApiException so
that we will adhere correctly to the Callback contract for onCompletion
specifying a valid (dummy) TopicPartition.

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

I'm also supposed to add:

The contribution is my original work and that I license the work to the project under the project's open source license.

[kirktrue]

@ayoukhananov - you're absolutely correct that there are conditions wherein tp wasn't initialized. This should fix that case too.

[kirktrue]

@hachikuji - would you be willing to assign a reviewer for this PR?

The failing tests look like they're related to KRaft tests, so I don't think they're related.

[kirktrue]

@cmccabe - could you take a look at this and/or assign a reviewer for this? Thanks!

[cmccabe]

If I understand correctly, the PR makes up a fake partition so that TopicPartition can be non-null in the callback. I don't think that this is the right thing to do. Why not change the JavaDoc for the callback to indicate that TopicPartition can be null if the method fails before it gets assigned?

[kirktrue]

Thanks @cmccabe for looking at this!

If I understand correctly, the PR makes up a fake partition so that TopicPartition can be non-null in the callback. I don't think that this is the right thing to do.

Yeah, I'm not crazy about it either. There was some "prior art" in the codebase where the same thing was done (ProducerInterceptors.onSendError) and the existence of RecordMetadata.UNKNOWN_PARTITION suggested that it might be permissible for these kinds of cases.

Why not change the JavaDoc for the callback to indicate that TopicPartition can be null if the method fails before it gets assigned?

My interpretation was that was akin to changing the interface of the Callback API and could cause some NullPointerException cases in users' code.

I'm happy to make the change to the JavaDoc, as suggested.

Thanks!

[ijuma]

@kirktrue Given how it works for the producer interceptor, it seems reasonable to do something similar for the consumer. @cmccabe do you agree?

[kirktrue]

Closing this PR in favor of #11689.

[philipnee]

@kirktrue Given how it works for the producer interceptor, it seems reasonable to do something similar for the consumer. @cmccabe do you agree?

let's move the conversation to #11689