[GitHub] [kafka] C0urante commented on a change in pull request #10503: KAFKA-9988: Suppress uncaught exceptions in log messages during task shutdown

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

C0urante commented on a change in pull request #10503:
URL: https://github.com/apache/kafka/pull/10503#discussion_r609882468



##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java
##########
@@ -185,8 +185,15 @@ private void doRun() throws InterruptedException {
 
             execute();
         } catch (Throwable t) {
-            log.error("{} Task threw an uncaught and unrecoverable exception. Task is being killed and will not recover until manually restarted", this, t);
-            throw t;
+            if (!stopping) {
+                log.error("{} Task threw an uncaught and unrecoverable exception. Task is being killed and will not recover until manually restarted", this, t);
+                throw t;
+            } else {
+                log.warn("{} During stop, task threw an uncaught and unrecoverable exception: {}.", this, t.getMessage());

Review comment:
       Nit 1: "During stop" may be interpreted as "During an invocation of `Task::stop`", which isn't necessarily the case here if, for example, shutdown is scheduled but the task fails during a call to `put` or `poll` before `stop` can even be invoked.
   
   Nit 2: Also, this might be more of a personal thing, but I'm wondering if "unrecoverable" really adds much here since, even if the task were to somehow "recover", it'd still just shut down.
   
   Nit 3: I think it's still valuable to preserve the entire stack trace instead of only revealing with `DEBUG`-level logging. Any time a task throws an uncaught exception, it should be cause for concern, as there may be issues with the quality of the connector, the health of the Kafka cluster, or the configuration of the worker. And since task restarts have to be manually triggered, it doesn't seem likely that this'd lead to logs being flooded with stack traces. 
   
   WDYT about something like `log.warn("{} After being scheduled for shutdown, task threw an uncaught exception.", this, t);`?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org