[LI-HOTFIX] catch throwable instead of exception from user callback o…

[xiowu0]

…n completeFutureAndFireCallbacks

TICKET = KAFKA-10806
LI_DESCRIPTION = When kafka producer tries to complete/abort a batch, producer invokes user callback. However, "completeFutureAndFireCallbacks" only captures exceptions from user callback not all throwables. An uncaught throwable can prevent the batch from being freed.

EXIT_CRITERIA = TICKET [KAFKA-10806]

[smccauliff]

It's generally a bad idea to catch throwable. Why is this needed?

[xiowu0]

Venice team encounters an error that their producer stuck on waiting for memory. The actual error happened several days ago (the producer continues to function after the initial error). java.lang.OutOfMemoryError is thrown from the user callback, and it is not caught by the exception handlers. As a result, the batch is not cleaned up, and leave the Kafka producer in an unstable state. Since Kafka client doesn't have control over the user callback, and the callback is executed in the kafka client thread, it is ok to isolate user errors from kafka threads. What do you think?

[smccauliff]

The only safe action to take when OOM happe s is to exit.

[xiowu0]

The best approach is for users to handle these errors in the callback, which Venice team tries to fix. On our side, I think catching throwables and not leave kafka client in an unstable state have some benefits. For example, if users eventually detect the error by other means, they can still gracefully flush pending records and shut down kafka producer. In addition, some errors may not require close/restart kafka producers.

[smccauliff]

No, they can't gracefully shutdown.

Anything they need to do probably requires memory allocation which is now likely to fail. We also can't make any guarantees about the behavior of our clients after OOM for the same reasons.

[smccauliff]

The JDK documentation itself says you should not be doing this. See:

https://docs.oracle.com/javase/7/docs/api/java/lang/Error.html

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch. Most such errors are abnormal conditions. The ThreadDeath error, though a "normal" condition, is also a subclass of Error because most applications should not try to catch it.

[radai-rosenblatt]

there's no way to recover from OutOfMem. its not guaranteed that the OOM will be thrown on the same code/thread thats consuming the memory.

the only thing you can do on OOM is die, hence i doubt its worth catching.
worse, catching this will mask the OOM?

perhaps its better if whatever needs to be cleaned up is cleaned up in a finally block, instead of catching throwables?

[xiowu0]

Ok, the issue here is the error is invisible from users since the callback is executed on the kafka client's thread. I am ok not to swallow the error (throwable), but we might need a better way to propagate the error to the user so that user can act appropriately. In addition, the error happens when executing user code, any error could happen, and Kafka client is not responsible for handling these errors.

[radai-rosenblatt]

throw it out of the next poll() call ?

[xiowu0]

this is Kafka producer. A simple solution is to catch exceptions and throwable, log the error if producer sees the exceptions, but close the sender thread (unrecoverable) if producer sees throwables. However, the con of this approach is user still don't see the throwable. Carrying the throwable to next "send" looks like a hack to me.