TODO error case in broker recieve

[banks]

Hi sorry this is a lame issue but reading code I came accross:

https://github.com/Shopify/sarama/blob/master/broker.go#L392-L395

Seems like quite a major point to be left as a TODO? In fact I think the same applies to any of the 3 places in that loop that return error and continue - they left socket with partial message read so surely all subsequent reads will also fail until you kill the connection.

Perhaps in practice something higher up the stack does that but it seems like a pretty critical issue that must come up in practice.

I notice there was some discussion of the general error handling around broker connections in a few issues.

I guess maybe this is not a big deal since most people are only using the QueueMessage interface?

I'm currently trying to figure out how to implement a Kafka producing daemon that has some pretty specific requirements around error handling that Sarama doesn't yet support, but my two cents on this are that you certainly should fail harder and close connection in those cases.

[banks]

Oh and I forgot to say: thanks for open sourcing this lib - it seems generally solid and well thought out!

[eapache]

Ya, in practice it ended up being handled up the stack. If anything causes an error in responseReceiver(), that error gets bubbled up the stack to the original caller of the public method (Fetch, Produce, etc) and in all of those cases the caller reacts by giving up on the broker (e.g. https://github.com/Shopify/sarama/blob/master/consumer.go#L192 or https://github.com/Shopify/sarama/blob/master/client.go#L304).

The broker error-handling API is one of my sore points and something I wish I could revisit somehow without totally rewriting everything... oh well :)

The current producer design is somewhat problematic. I have a new design that should be much better, but since I wrote it I have been unable to find the time to tidy it up, test it and merge it: #132. I will try again to find some time for this, but you may find it useful if you're running into limitations with the current producer. No guarantees etc. but it should work in all the same cases the current one does.

[eapache]

Of course, if you try the new design and you do have any questions or issues with it please let us know, it will give us a better idea of what's left to do on it before it's fully ready to merge.

[banks]

Thanks Evan

I will take a look at the new producer when I get a chance.

On 10 Oct 2014, at 14:13, Evan Huus notifications@github.com wrote:

Of course, if you try the new design and you do have any questions or issues with it please let us know, it will give us a better idea of what's left to do on it before it's fully ready to merge.

—
Reply to this email directly or view it on GitHub.

[eapache]

Closing this as I think I managed to answer the primary question.