search

heetch / felice

Felice is a nascent, opinionated Kafka library for Go, in Go
MIT License
20 stars 1 forks source link

Remove sarama cluster dependency #45

Closed yaziine closed 5 years ago

yaziine commented 5 years ago

Since sarama-cluster has been deprecated, this PR uses the Sarama's Consumer Group API.

Before, the way Kafka was consuming messages was configured through the cluster.ConsumerMode option saying that we want a go routine for each topics/partitions.

Now, it is already handled by Sarama. There is a ConsumerGroupHandler for the group which runs a separate goroutine (ConsumeClaim function) for each topics/partitions.

This fixes #39.

yaziine commented 5 years ago

@asdine 🆙

yaziine commented 5 years ago

I wish the tests were less internal so that we could keep exactly the same tests and thereby have better confidence that behaviour hasn't changed

@rogpeppe IIUC you mean that we should have more integration test? But it involves having a kafka instance spawned etc...

yaziine commented 5 years ago

@asdine No changes required regarding kafka-go.

yaziine commented 5 years ago

@rogpeppe before merging it I want to be sure that your comments are well addressed.

rogpeppe commented 5 years ago

@asdine

@rogpeppe IIUC you mean that we should have more integration test? But it involves having a kafka instance spawned etc...

This is an area that needs some serious consideration, I think. Maybe there's room for a decent quality Kafka fake implementation (as opposed to a mock) that can be run in-process.

From my point of view, a lot of the value of tests isn't just that they give you confidence that your code works now, but also give confidence across future code refactoring such as what we're doing in this PR. So when I see tests being removed or significantly changing as a result of what should be a behaviour-preserving refactor, it makes me concerned that perhaps the tests could be better.

This isn't your fault, of course, but is something that's perhaps worth pondering in the future.

yaziine commented 5 years ago

I merge the PR. I created an issue feel free to continue the discussion there.

asdine commented 5 years ago

@rogpeppe I'll create an issue regarding that 👍

asdine commented 5 years ago

@rogpeppe https://github.com/heetch/felice/issues/48