Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#239 integrate sarama consumer group #260

Merged
merged 32 commits into from
Jul 9, 2020
Merged

#239 integrate sarama consumer group #260

merged 32 commits into from
Jul 9, 2020

Conversation

frairon
Copy link
Contributor

@frairon frairon commented Jul 8, 2020

  • integrate sarama consumer group
  • remove sarama-cluster dependency

frairon and others added 28 commits March 19, 2020 13:52
Co-authored-by: R053NR07 <jan.bickel@lovoo.com>
updated readme for configuration, added changelog
Open Storage in PartitionTable when performing Setup
return trackOutput if stats are nil
added lots of godoc
fixed many linter errors
added Open call when creating storage
added lots of godoc, fixed many linter errors, added Open call when c…
added strings to streams helper
Add Backoff when restartable CatchupForever is executed
Panic documentation & Migration guide
R053NR07
R053NR07 previously approved these changes Jul 9, 2020
@frairon frairon merged commit 075bc83 into master Jul 9, 2020
@frairon frairon deleted the consumer-group branch July 9, 2020 12:05
@fkarakas
Copy link

fkarakas commented Nov 19, 2020

Hello @frairon,
I have a question : Why did u remove confluent lib for sarama ?
Because the official library is maintained and updated by confluent so more "safe" regarding its maintenance in the future ...
thanks

@frairon
Copy link
Contributor Author

frairon commented Nov 19, 2020

Hi @fkarakas, you mean the support for the C implementation of kafka (I think it was based on librdkafka?), right?

The thing was, that this implementation follows a completely different logic to rebalance and receive messages. In the "old" goka version, we had to do quite some magic to support both sarama and librdkafka, so it was very hard to understand, maintain, and it behaved flaky in an unstable kafka cluster. So we decided to simplify the logic and get rid of it for now.
If this is something that people still need, I think we can somehow get it working again by trying to bridge its behavior to the new sarama logic of the consumer group. But haven't looked into it at all so far.
If you are willing to work on that, PRs are always welcome :D

@fkarakas
Copy link

fkarakas commented Nov 19, 2020

Thanks, if i understand correctly it was difficult to make an abstraction in the goka API which can both work with confluent and sarama because internally they work differently

@frairon
Copy link
Contributor Author

frairon commented Nov 19, 2020

Yep exactly, and the point of the refactoring was to simplify things, get rid of sarama-cluster. Are you using the confluent api?

@fkarakas
Copy link

Yes we are using the confluent lib, as you mentioned it is based on the C librdkafka so it is not a full GO lib which can be a stopper for go purist, but we decided to use it because it is confluent official lib for GO so we think it is "safer" regarding its maintenance/upgrade in the future ...
And in kafka, a big part of the "logic" is implemented in the client side instead of the server side, so it was another +1 in the choice of confluent-kafka-go. This was our conclusion.

@frairon
Copy link
Contributor Author

frairon commented Nov 19, 2020

That sounds like a reasonable choice. So is this a blocker for you to upgrade to a new goka version? And would you have the resources to look into it?

@R053NR07
Copy link

R053NR07 commented Dec 4, 2020

Hey @fkarakas,
as you said the "pure GO" implementation was the main driver for the decision. CGO always was and is a problem for us, we had to take extra effort to use goka since this was a requirement. Also goka internally we focused mainly on sarama and supporting confluentlib would have been extra effort. We decided to focus on sarama since it is widely used in the go world for working with kafka and we did not see any major limitations or drawbacks when using it.

Regarding the support. Sure librdkafka may get longer support but even if sarama would be deprecated today we would be able to integrate and switch to librdkafka; for us the effort would be almost the same then designing goka with interchangeable kafka libs.

But still. If that's a blocker for you and others we can talk about changing this in future releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants