Confluent-kafka-go: Using proper time durations

[pavelnikolov]

Description

At the moment all of your duration parameters are of type int (e.g. func (p *Producer) Flush(timeoutMs int) int). I guess this has been done as an effort to make the Go library as close as possible to the underlying library. Unfortunately, this is not very Go-friendly way of using time. In every single other project Go developers would use time.Duration so the above example would become func (p *Producer) Flush(timeout time.Duration) int. This would allow us to invoke it like this:

p.Flush(15 * time.Second)

instead of the way it is in your example in the README.md:

p.Flush(15 * 1000)

The underlying library would still receive milliseconds, but this wouldn't affect Go developers.

As a Go developer I would recommend using time.Duration for better readability and sticking to the Go conventions. If you agree, I'd be happy to send a PR with this change.

[edenhill]

You are absolutely right, we should not have used int timeouts.
There's been some discussion on whether we should move to contexts which provides both timeouts and cancellations, but it is not currently clear how to make cancellations work with the underlying C library - there's definately room for some research in this area.

As for your suggestion, I'm suspecting that would be a breaking change for existing applications, so it would need to be timed with a future 1.0.0 release.

[pavelnikolov]

I'm willing to improve the Go client and would be happy to discuss some ideas with you and contribute to the project. Indeed, using contexts would be ideal.

[pavelnikolov]

... so it would need to be timed with a future 1.0.0 release

When you create a 1.0.0 branch, I'd be happy to contribute to it, even if it's not getting merged any time soon.

Fixing the version of the Go client with the version of the C library makes it very hard to introduce changes like this.

[edenhill]

We very much welcome discussions and contributions for improving the client :+1:

The version sync between Go and librdkafka is not strictly fixed, librdkafka is for example at 0.11.3 while Go is still at 0.11.0, and it is possible for the reverse to happen as well. But we aim to keep them somewhat in synch when possible.
The upcoming 0.11.4 release of librdkafka will also be joined with 0.11.4 releases of the Go, Python and .NET clients.

As for API breakage, we try our hardest not to break the API unless required as not to mess up existing users. However, we do plan to release 1.0.0 in Q2 and that would allow us to break the API.

I think the best approach for this enhancement is to put the effort into making it based on context, since that is the end goal.
librdkafka currently does not support cancellation of blocking calls, but there is demand for such an API from other venues as well (such as Ctlr-C in Python), so we'll add an API to librdkafka that let's you do this. It would be useful to have the Go context requirements when we do this.

[pavelnikolov]

Well, adding context support can be a non-breaking change in the Go world. Many projects just add the same method(s) with Context suffix. For example, initially there was only Query method in the database/sql package:

func (db *DB) Query(query string, args ...interface{}) (*Rows, error)

and later QueryContext was added:

func (db *DB) QueryContext(ctx context.Context, query string, args ...interface{}) (*Rows, error)

Since you already have existing consumers to support, I suggest that you introduce *Context(ctx context.Context, ...) methods where needed. This would be a non-breaking change (in Go).

[edenhill]

That looks good.

Would you like to look into context:ualizing Pool(), figuring out what goes into the Go code and what is needed from the underlying C code?

[pavelnikolov]

I'll take a look soon and will get back to you...

[pavelnikolov]

I pushed #149 as an initial prototype. I'd need some requirements from you, before I can continue with other things.