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/06/30 23:05:28 UTC

[GitHub] [kafka] kirktrue opened a new pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

kirktrue opened a new pull request #10951:
URL: https://github.com/apache/kafka/pull/10951


   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)
   


-- 
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] philipnee commented on pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
philipnee commented on pull request #10951:
URL: https://github.com/apache/kafka/pull/10951#issuecomment-1027466090


   > @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 


-- 
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] cmccabe commented on pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #10951:
URL: https://github.com/apache/kafka/pull/10951#issuecomment-887023640


   This doesn't seem correct. You're making up a fake partition just so that `TopicPartition` can be non-null in the callback. Why not change the JavaDoc for the callback to indicate that `TopicPartition` can be null if the method fails before it gets assigned?


-- 
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] ijuma commented on pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10951:
URL: https://github.com/apache/kafka/pull/10951#issuecomment-1027303692


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


-- 
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] kirktrue commented on pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
kirktrue commented on pull request #10951:
URL: https://github.com/apache/kafka/pull/10951#issuecomment-1027464655


   Closing this PR in favor of #11689.


-- 
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] cmccabe edited a comment on pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
cmccabe edited a comment on pull request #10951:
URL: https://github.com/apache/kafka/pull/10951#issuecomment-887023640


   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?


-- 
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] ayoukhananov commented on a change in pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
ayoukhananov commented on a change in pull request #10951:
URL: https://github.com/apache/kafka/pull/10951#discussion_r662049082



##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
##########
@@ -982,8 +982,12 @@ private void throwIfProducerClosed() {
             // for other exceptions throw directly
         } catch (ApiException e) {
             log.debug("Exception occurred during message send:", e);
-            if (callback != null)
-                callback.onCompletion(null, e);
+            // producer callback will make sure to call both 'callback' and interceptor callback
+            Callback interceptCallback = new InterceptorCallback<>(callback, this.interceptors, tp);

Review comment:
       I am not sure it covers the case KafkaException is thrown in line 913.
     throw new KafkaException("Producer closed while send in progress", e);
     
     Which means tp still not initialised and has the null value.




-- 
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] kirktrue commented on pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
kirktrue commented on pull request #10951:
URL: https://github.com/apache/kafka/pull/10951#issuecomment-876583460


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


-- 
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] kirktrue commented on pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
kirktrue commented on pull request #10951:
URL: https://github.com/apache/kafka/pull/10951#issuecomment-875058547


   @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.


-- 
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] kirktrue commented on pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
kirktrue commented on pull request #10951:
URL: https://github.com/apache/kafka/pull/10951#issuecomment-872470098


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


-- 
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] ayoukhananov commented on a change in pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
ayoukhananov commented on a change in pull request #10951:
URL: https://github.com/apache/kafka/pull/10951#discussion_r662049082



##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java
##########
@@ -982,8 +982,12 @@ private void throwIfProducerClosed() {
             // for other exceptions throw directly
         } catch (ApiException e) {
             log.debug("Exception occurred during message send:", e);
-            if (callback != null)
-                callback.onCompletion(null, e);
+            // producer callback will make sure to call both 'callback' and interceptor callback
+            Callback interceptCallback = new InterceptorCallback<>(callback, this.interceptors, tp);

Review comment:
       I am not sure it covers the case KafkaException is thrown in line 913.
     throw new KafkaException("Producer closed while send in progress", e);
     
     Which means tp still not initialised and has the null value.




-- 
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] kirktrue commented on pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
kirktrue commented on pull request #10951:
URL: https://github.com/apache/kafka/pull/10951#issuecomment-887627439


   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!
   


-- 
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] kirktrue closed pull request #10951: KAFKA-12841: NPE from the provided metadata in client callback in case of ApiException

Posted by GitBox <gi...@apache.org>.
kirktrue closed pull request #10951:
URL: https://github.com/apache/kafka/pull/10951


   


-- 
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