MINOR: Update docs for producer callbacks to reflect current behaviour#11470
MINOR: Update docs for producer callbacks to reflect current behaviour#11470soceanainn wants to merge 1 commit intoapache:trunkfrom soceanainn:fix-kafka-client-producer-docs
Conversation
|
cc @junrao as you were involved in earlier PRs / threads on this topic |
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@soceanainn : Thanks for the PR. Left a comment below.
| * A callback method the user can implement to provide asynchronous handling of request completion. This method will | ||
| * be called when the record sent to the server has been acknowledged. When exception is not null in the callback, | ||
| * metadata will contain the special -1 value for all fields except for topicPartition, which will be valid. | ||
| * and the exception is a subclass of ApiException, metadata will be null. For all other exceptions, metadata will |
There was a problem hiding this comment.
It seems that the fix in #5798 was incorrect. The changes for the comment were intended for InterceptorCallback.onComplete. For Callback.onCompletion, currently, if there is an exception, it seems that metadata will always be null.
For now, we could probably just revert the comment on Callback.onCompletion.
It's probably useful to make the behavior consistent between Callback.onCompletion and InterceptorCallback.onComplete. We could file a jira to track that since it probably needs a KIP.
There was a problem hiding this comment.
I don't think metadata will always be null, as the updated metadata will sometimes be passed to the user callback as far as I can see from the InterceptorCallback implementation (see here).
I believe we have been seeing a mixture of null metadata + placeholder metadata in our Kafka deployment (v2.3.1), which has caused some issues for my team.
There was a problem hiding this comment.
@soceanainn : Thanks for the comment. You are right. It seems that we are generating metadata inconsistently. It seems that if the send() call throws an ApiException, we return a null metadata. Otherwise, the callback will always get a placeholder metadata when exception is not null, even if the exception is of ApiException.
There was a problem hiding this comment.
What do you suggest? Should the docs be updated, or perhaps we should simply remove lines 1001-1002, as this would make the behaviour consistent across Callback.onCompletion and InterceptorCallback.onComplete, and in line with the current documented behaviour
|
@junrao I've created a second PR #11482 that updates the behaviour when ApiException is thrown to align with other exceptions / documented behaviour. Depending on whether you think we should change the current behaviour to align with the docs, or change the docs to align with the current behaviour, I will close one of these two PRs and we can continue the discussion in the relevant PR. |
|
@soceanainn : I agree that it makes sense to change the behavior of ApiException to be more consistent with other places. Since it's a user facing change, we probably need to do a KIP. |
|
I'll close this PR in favor of #11482 and we can discuss the creation of the KIP there |
2 participants
Originally, Callback would return a null metadata value when an error occurred.
This was partially changed by KAFKA-3303, where in some cases Callback would return an 'empty' metadata. In this empty metadata TopicPartition is set correctly but all other fields are set as
-1.The docs were later updated by KAFKA-7412, but it incorrectly states that Callback will always return this 'empty' metadata when an error occurs. However in the case of any exceptions that are a subclass of ApiException, Callback will still return a null value (see here).
This change aims to clarify the docs to accurately reflect the behaviour of producer callbacks.
Committer Checklist (excluded from commit message)