[GitHub] markusthoemmes commented on a change in pull request #4080: Catch kafka producer exceptions and recreate the producer

[GitBox <gi...@apache.org>]

markusthoemmes commented on a change in pull request #4080: Catch kafka producer exceptions and recreate the producer
URL: https://github.com/apache/incubator-openwhisk/pull/4080#discussion_r227712669
 
 

 ##########
 File path: common/scala/src/main/scala/whisk/connector/kafka/KafkaProducerConnector.scala
 ##########
 @@ -55,12 +55,16 @@ class KafkaProducerConnector(
 
     Future {
       blocking {
-        producer.send(record, new Callback {
+        Try(producer.send(record, new Callback {
           override def onCompletion(metadata: RecordMetadata, exception: Exception): Unit = {
             if (exception == null) produced.success(metadata)
             else produced.failure(exception)
           }
-        })
+        })).recover {
+          case e: Exception =>
+            logging.error(this, e.getMessage)
+            produced.failure(e)
+        }
 
 Review comment:
   Since you don't do anything with the `Try` value, a simple `try/catch` would suffice here and prevent unnecessary allocations.
   
   The logging is unnecessary I believe, since this will then be fed into the error handling logic below, which logs failures by default as well. The logline would be redundant.
   
   Also, to be absolutely sure we're not creating a new race condition here, please change all the `success`/`failure` calls to `trySuccess`/`tryFailure`. That'll make sure that even if the Callback is executed on a thrown exception (unlikely but better be defensive) the promise resolving code isn't throwing another exception.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services