Handling 0 as possible required ack value and InvalidRequiredAcks errors

[ppatierno]

This PR fixes #174 and #175.

It first adds handling the 0 value as a possible valid required ack and the handling of a potential InvalidRequiredAcks when required ack is set to a value different from allowed -1, 0, 1.

Because required ack = 0 means that the producer doesn't have to expect any response from the broker because it means "no ack", the waitResponse doesn't need to be called otherwise it drives to a timeout error having the producer waiting for a response which will never come.

[achille-roussel]

Don't we have to check that this is a produce request? do is used for all operations, but "required acks" is only employed when send messages isn't it?

[ppatierno]

I totally agree with you but the waitResponse is done at connection level (so in the conn.go file) where as far as I can see from the code we don't have information if it's a producer or a consumer request (which is at higher level). We should do a refactoring for moving the waiting for the response at the upper layer.
Is that really a problem? I mean, the conn is always initialized with -1 as a default value so other requests will work out of the box (and for consumer we cannot change it).
Btw if you think there is a simple way to check for producer request, feedback are really appreciated :-)

[ppatierno]

Just adding that if the conn is there for representing a connection to the broker, the requiredAcks should not be an information at this so low level imho ... but it was already there.

[ppatierno]

We could thing to add a new waitResponse bool parameter for the do function passed by the writeOperation and readOperation used by higher level protocol operation for which we know if it's needed to wait a response or not.

[achille-roussel]

I think we're pretty much forced to handle it at this level since it's core to how the kafka protocol works (some requests have responses, some don't, currently we only support the former), and we use the Conn abstraction for all interactions with kafka brokers.

We don't know how the connection is being handled, but we do know what kafka API call is being made, so I think we can figure out whether the "required ack" applies or not based on that, can't we?

It's OK to modify internal APIs to support new use cases as well, if we need to pass more info down to this method then let's do it.

[ppatierno]

We don't know how the connection is being handled, but we do know what kafka API call is being made, so I think we can figure out whether the "required ack" applies or not based on that, can't we?

Which information inside conn.go provides the information about which API is called. Isn't it an information of higher level?

[achille-roussel]

The entry point for producing messages is here https://github.com/segmentio/kafka-go/blob/master/conn.go#L828

So this is where we have most of the context for deciding whether waiting for a ack is required or not.

I think the simplest is going to be copying the content of the do method into WriteCompressedMessages, in replacement for the call to writeOperation, and based on whether "required acks" are set or not we wait for the response.

What do you think about this?

[achille-roussel]

@ppatierno do you still have interest in pushing this through the finish line?

[mkorolyov]

@achille-roussel Hi, i can finish this, if you doesn't started to work on it yourself

[achille-roussel]

Catching up on this. First, apology for the delayed response, it's not due to a lack of interest in your contribution, just a lack of time.

@mkorolyov do you want to push this through the finish line or do you need someone to take over?

[mkorolyov]

@achille-roussel Hi, same thing with lack of time :)
Will finish this during next week
Cheers

[dominicbarnes]

I'm just checking back in on the status of this Pull Request. It looks like the functionality trying to be implemented is requiring 0 acks when writing messages. As far as I understand, the Kafka protocol here behaves differently from other operations in that the broker gives back no response, which the internals for v0.3 wouldn't handle gracefully.

That being said, v0.4 has already addressed this issue and allows for this with kafka.RequireNone, so if this functionality is desired I would recommend you upgrade to that and see if it fits your use-case, and submit PRs against that release rather than v0.3.

I will update our documentation to reflect this decision, and will close this PR along with the related issues #174 and #175. If you prefer to continue using v0.3, you are welcome to continue working with the master branch and submit a new PR that includes more test coverage for this functionality.