Make mock brokers and protocol packets available for outsider #570
Conversation
|
Hrm, a protocol subpackage has been on our 2.0 wishlist for a while (see https://github.com/Shopify/sarama/wiki/Ideas-that-will-break-backwards-compatibility). I'm not strictly against this PR, but I'm really more curious what case you're doing that the high-level mocks don't support? Are you actually using the low-level |
|
Why is introducing the protocol package considered a breaking change? Yes, strictly speaking the request and response structs are public and moving them to another package breaks the interface. But they have been unusable outside of sarama because of private encode/decode methods, so nobody could use them anyway, right? I would say de facto it is a non-breaking change. Answering you other question: we have our own implementation of consumer and offset_manager that have been living in our sarama clone. I want to stop maintaining the sarama clone and move the consumer and offset_manager implementations to where they are actually used. Just as I said. Very selfish :). |
|
They're not unusable, they are intended to be used with the If the current consumer/offset-manager don't do what you need, a less selfish approach would be to contribute that functionality upstream and just start using the sarama-provided ones... in the long run this might even be more selfish since then we'd take the maintenance burden for them :) |
|
@eapache when I was adding missing features I was stupid enough to rewrite the Consumer and OffsetManager in a slightly different way. I also slightly changed their API. Since then you fixed probably all of the issues I have had with them, but it is not easy for me to move to the original sarama Consumer and OffsetManager. As a step in this direction I wanted to move custom logic away from sarama, but for that I need to be able to test it using Sarama's mocks. |
|
We're considering doing a breaking release, because some of the 0.9 protocol additions are not backwards compatible. As part of this we could also extract out the protocol stuff into a subpackage. |
|
@wvanbergen do I get this right that Sarama v2 compatible with Kafka 0.9 won't be able to talk to Kafka 0.8? |
|
It should still be able to for most/all API calls, with a potential exception of the offset manager. |
|
Alas that would not work for us, we do use the offset manager with Kafka 0.8 so if we switch to Sarama v2 we won't be able to support both Kafka 0.8 and Kafka 0.9. |
|
The offset manager may work as well with both versions, but we'd have to investigate some more before we can be sure. The issues: the current offset manager doesn't work with 0.9 for unknown reasons, and a bunch of error/protocol names have been changed compared to Kafka 0.8. Could be fixable. |
|
@wvanbergen thanks for sharing, I appreciate that. |
|
@wvanbergen check out #585. Looks like it won't be necessary to make backwards incompatible changes after all. |
|
I'm trying to write some tests right now that need the mock broker as well. So it isn't that selfish @horkhe I'm trying to upgrade some code to use the latest version of sarama and part of that code uses a Client, which is an interface and can be mocked except for the fact that a few of the functions return a real *Broker. These tests are pretty difficult to fix/upgrade without a mock broker. |
|
Hmm, I wonder if long-term the better solution might just be to make |
|
Well, people would still need to come up with some sort of mock broker implementation. And the one that comes with Sarama would satisfy many of us. So event with a Broker interface it would still make sense to make the sarama mock broker implementation public, I think. |
|
Hey @eapache any chance of getting this to the master? If you do not think that it is worth having then let's just close this PR. |
|
Urgh, I dislike all the types this adds to our public API, but I'm also not super-keen on doing a 2.0 just so we can package everything up properly, and I do understand the problem you're trying to solve. I think I'm OK with this PR if the documentation for the various mock types mentions that you probably don't need them / shouldn't use them unless you're doing low-level testing? @wvanbergen do you have a strong opinion here? |
| @@ -36,7 +36,7 @@ type requestHandlerFunc func(req *request) (res encoder) | |||
| // | |||
| // It is not necessary to prefix message length or correlation ID to your | |||
| // response bytes, the server does that automatically as a convenience. | |||
| type mockBroker struct { | |||
| type MockBroker struct { | |||
There was a problem hiding this comment.
godoc for this needs to s/mockBroker/MockBroker/
|
@eapache thanks man! If you plan on going to GopherCon this year I will be glad to meet you guys there and treat you with some beer :) |
|
I'll take Willem's lack of response as "no strong opinion" so: update the godoc to make it explicit these types are for low-level testing and aren't normally needed, and I'll merge this. |
|
Sorry, I was sick the last week so I missed this. But yeah - if this solves a problem for you I am OK with merging this. |
|
@eapache @wvanbergen I have updated the comments. |
Make mock brokers and protocol packets available for outsider
This is a very selfish PR, I am probably one of the very few people (if not the only one) who needs this, so if you think that it makes no sense for you then just close this PR immediately.
Basically I need MockBroker and Mock response types to be available outside of Sarama to reuse in our tests. I also wanted to move them to
saram/mockpackage, but that would require making the encoder type public that would collide with the existing public Encoder type. A solution to that would be to move all request/response marshaling/unmarshaling logic to a dedicated package e.g.sarama/protoorsarama/protocol, but that seemed to be too much to ask...