You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/11/05 13:47:37 UTC
[GitHub] [kafka] soceanainn opened a new pull request #11470: MINOR: Update docs for producer callbacks to reflect current behaviour
soceanainn opened a new pull request #11470:
URL: https://github.com/apache/kafka/pull/11470
Originally, Callback would return a null metadata value when an error occurred.
This was partially changed by [KAFKA-3303](https://issues.apache.org/jira/browse/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](https://issues.apache.org/jira/browse/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](https://github.com/apache/kafka/blob/3.1/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java#L1002)).
This change aims to clarify the docs to accurately reflect the behaviour of producer callbacks.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #11470: MINOR: Update docs for producer callbacks to reflect current behaviour
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11470:
URL: https://github.com/apache/kafka/pull/11470#discussion_r745176383
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/Callback.java
##########
@@ -25,10 +25,12 @@
/**
* 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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] soceanainn commented on pull request #11470: MINOR: Update docs for producer callbacks to reflect current behaviour
Posted by GitBox <gi...@apache.org>.
soceanainn commented on pull request #11470:
URL: https://github.com/apache/kafka/pull/11470#issuecomment-961908428
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] soceanainn commented on pull request #11470: MINOR: Update docs for producer callbacks to reflect current behaviour
Posted by GitBox <gi...@apache.org>.
soceanainn commented on pull request #11470:
URL: https://github.com/apache/kafka/pull/11470#issuecomment-961908428
cc @junrao as you were involved in earlier PRs / threads on this topic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] soceanainn commented on pull request #11470: MINOR: Update docs for producer callbacks to reflect current behaviour
Posted by GitBox <gi...@apache.org>.
soceanainn commented on pull request #11470:
URL: https://github.com/apache/kafka/pull/11470#issuecomment-965027490
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] soceanainn commented on a change in pull request #11470: MINOR: Update docs for producer callbacks to reflect current behaviour
Posted by GitBox <gi...@apache.org>.
soceanainn commented on a change in pull request #11470:
URL: https://github.com/apache/kafka/pull/11470#discussion_r745122437
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/Callback.java
##########
@@ -25,10 +25,12 @@
/**
* 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
Review 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](https://github.com/apache/kafka/blob/3.1/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java#L1387-L1390)).
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on pull request #11470: MINOR: Update docs for producer callbacks to reflect current behaviour
Posted by GitBox <gi...@apache.org>.
junrao commented on pull request #11470:
URL: https://github.com/apache/kafka/pull/11470#issuecomment-965642928
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] soceanainn commented on a change in pull request #11470: MINOR: Update docs for producer callbacks to reflect current behaviour
Posted by GitBox <gi...@apache.org>.
soceanainn commented on a change in pull request #11470:
URL: https://github.com/apache/kafka/pull/11470#discussion_r746457370
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/Callback.java
##########
@@ -25,10 +25,12 @@
/**
* 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
Review comment:
What do you suggest? Should the docs be updated, or perhaps we should simply remove lines [1001-1002](https://github.com/apache/kafka/blob/3.1/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java#L1001-L1002), as this would make the behaviour consistent across `Callback.onCompletion` and `InterceptorCallback.onComplete`, and in line with the current documented behaviour
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] soceanainn commented on pull request #11470: MINOR: Update docs for producer callbacks to reflect current behaviour
Posted by GitBox <gi...@apache.org>.
soceanainn commented on pull request #11470:
URL: https://github.com/apache/kafka/pull/11470#issuecomment-965658138
I'll close this PR in favor of #11482 and we can discuss the creation of the KIP there
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] soceanainn closed pull request #11470: MINOR: Update docs for producer callbacks to reflect current behaviour
Posted by GitBox <gi...@apache.org>.
soceanainn closed pull request #11470:
URL: https://github.com/apache/kafka/pull/11470
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #11470: MINOR: Update docs for producer callbacks to reflect current behaviour
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #11470:
URL: https://github.com/apache/kafka/pull/11470#discussion_r745109562
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/Callback.java
##########
@@ -25,10 +25,12 @@
/**
* 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
Review comment:
It seems that the fix in https://github.com/apache/kafka/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org