AsyncProducer.Close() doesn't flush if conf.Producer.Return.Errors is false

[hchargois]

Versions

Sarama Version: fd49817 (latest as of today)

Problem Description

The documentation says about Close():

Close shuts down the producer and flushes any messages it may have buffered.

and about Errors():

Alternatively, you can set Producer.Return.Errors in your config to false, which prevents errors to be returned.

To me, this means that if I don't care about errors, I can simply set Return.Errors = false with no ill-effects, then when I'm finished Close() the AsyncProducer and I expect it to block until it is flushed (as opposed to AsyncClose().

This is not the case. Close() is simply a call to AsyncClose() followed by blocking on the Errors() channel, if and only if Return.Errors is true, if it is false then it simply won't block and won't flush if the program exits shortly after.

This is definitely not what I expect from the documentation. In my opinion, Close() should be implemented in such a way that it does actually block, regardless of the values of (what one should expect to be) orthogonal configuration variables.

Alternatively, a big caveat should be made clear in the doc: if Return.Errors is false then Close() won't block, you have to set Return.Successes to true and block on that yourself, and if Return.Successes is false also, then there is no way to flush the producer reliably.

[eapache]

This doesn't seem like a consistent position to me? If you really don't care about errors then it shouldn't matter whether Close flushes reliably because at worst it drops messages and you've already said you don't care about that.

I will definitely improve the documentation to make the behaviour clearer, but I'm not sure I really understand the use case you're trying to fulfill here with Return.Errors false but still expecting Close to block.

[hchargois]

It's definitely a corner case but I don't think it's wildly inconsistent to not care about errors enough to want to act on them, but still expect a library to at least try.

When you have a reliable Kafka cluster and network and you want to write some one-shot low-volume tool, it's not unthinkable to assume that if you just blindly send, either everything will be OK or everything will fail because you got something wrong in your code. You don't expect to lose a few messages at exit, especially if it looks like you close everything properly. This assumption, that I had to debug in someone else's code, is why this ticket exist :)