Broker should be an interface

[toddpalino]

Versions

Please specify real version numbers or git SHAs, not just "Latest" since that changes fairly regularly.
Sarama Version: 70f6a70
Kafka Version: any
Go Version: any

Configuration

N/A - source enhancement

Logs

N/A

Problem Description

In rewriting an application that depends on Sarama and writing tests, I'm finding that while it's easy to mock out the client for testing, as it a defined interface, it's not as easy with the Broker object. Unfortunately, the usage of Sarama has me depending on the Broker class a bit and calling the exposed methods directly. This means that in order to test, I'm going to have to write a shim interface for both Client and Broker so that I can swap in a test mock when needed.

I'm happy to do the work and submit a PR for the changes to Broker. I wanted to open the issue as well as a placeholder for the work and to make sure that there's nothing I'm missing that would preclude making this change.

[eapache]

There was some discussion of this a while ago at #570.

Off the top of my head, this would be a breaking API change?

[toddpalino]

I guess the question is would it actually be breaking? Yes, it would be an API change - Client, for example, would need to be changed to handle interfaces instead of the Broker struct. However, the Broker struct has no exposed fields, which means it can only be used for its methods (or to pass back in to something that can use the unexposed fields). The type name for the struct could remain Broker and the interface could be named something else (like BrokerInterface or AbstractBroker).

[eapache]

Creating a separate public type for the interface would definitely be a breaking change since code like

var broker *Broker
var err error
broker, err = client.Leader("foo", 123)

would fail to compile when the return value of Leader changed from (*Broker, error) to (BrokerInterface, error).

However, I haven't immediately been able to come up with code that would break if we just changed the struct to type Broker interface {} with a private implementing struct. Usage would be really weird since we'd be taking a pointer to an interface everywhere, and intuitively I would expect something to break, but I haven't actually been able to find it yet.

[eapache]

Oh, it would break because go won't dereference pointers to interfaces automatically, you get stuff like leader.ID undefined (type *Broker is pointer to interface, not interface)

[toddpalino]

Oh yeah, that's right. There's not really any way around that one. It's probably still worthwhile to do this for the next major change when breaking the interface will be OK, but at least for the time being I've worked around this for my own personal use by wrapping an interface around Sarama so I can test.

[eapache]

Ya, we're accumulating a bunch of stuff for when we end up doing a new major release. Glad to know it wasn't a blocker for the time being.

https://github.com/Shopify/sarama/wiki/Ideas-that-will-break-backwards-compatibility