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

Change kafka library to kafka-go #4692

Closed
sansmoraxz opened this issue Jun 16, 2023 · 13 comments · Fixed by kedacore/keda-docs#1187 or #4801
Closed

Change kafka library to kafka-go #4692

sansmoraxz opened this issue Jun 16, 2023 · 13 comments · Fixed by kedacore/keda-docs#1187 or #4801
Assignees
Labels
feature-request All issues for new features that have not been committed to needs-discussion

Comments

@sansmoraxz
Copy link
Contributor

Proposal

Switch the kafka client library to use Kafka-go in place of sarama

Use-Case

Kafka-go has support for MSK IAM which can help address #3431

In addition to this it has much cleaner api, better documented and much better resource utilization.

Is this a feature you are interested in implementing yourself?

Maybe

Anything else?

kafka-go doesn't provide mock objects so any tests that need to be done has to be made through a kafka server (perhaps a seperate container can be provisioned for this).

In addition to this the supported auth types include:

  • Plain
  • SCRAM
  • MSK

I guess Oauthbearer and GSSAPI is as of yet not supported in kafka-go.

@sansmoraxz sansmoraxz added feature-request All issues for new features that have not been committed to needs-discussion labels Jun 16, 2023
@JorTurFer JorTurFer moved this from To Triage to To Do in Roadmap - KEDA Core Jun 16, 2023
@sbdtu5498
Copy link

LGTM! Will do this. /assign

@sansmoraxz
Copy link
Contributor Author

I was working on this. Perhaps we can collaborate.

@sansmoraxz
Copy link
Contributor Author

Just to put it out there, kafka-go does provide an interface for various sasl authentication mechanisms, you can check out here: https://github.com/segmentio/kafka-go/tree/main/sasl

Implementing any new sasl based authentication mechanisms will be easier as we don't need to wait for the library vendor (segment) to add support for them.

@zroubalik
Copy link
Member

Cool, @sansmoraxz are you able to tell me if we can use the same configuration & settings with this library?

    lagThreshold: '5'
    activationLagThreshold: '3'
    offsetResetPolicy: latest
    allowIdleConsumers: false
    scaleToZeroOnInvalidOffset: false
    excludePersistentLag: false
    version: 1.0.0
    partitionLimitation: '1,2,10-20,31'

@sansmoraxz
Copy link
Contributor Author

I am not sure if the version's something that's really required. The methods in it does have error as return types for api calls that can result in such. Others should work without issues.

@sbdtu5498
Copy link

I was working on this.

Extremely sorry, I had no idea you were working on it.

Just to put it out there, kafka-go does provide an interface for various sasl authentication mechanisms, you can check out here: https://github.com/segmentio/kafka-go/tree/main/sasl

Implementing any new sasl based authentication mechanisms will be easier as we don't need to wait for the library vendor (segment) to add support for them.

Sure that's a great advantage!

Perhaps we can collaborate.

Sure thing! If you are currently working on the implementation part or migrating current implementation from Sarama to Kafka-go. Let me know once you are done, I will add auth support for other SASL mechanism as well. Further I will take a look at Kafka-go, they have many auth providers missing. So maybe it'd be a good idea to add some in them too.

@sansmoraxz
Copy link
Contributor Author

My bad. Didn't mention it here before.

I will add auth support for other SASL mechanism as well. Further I will take a look at Kafka-go, they have many auth providers missing. So maybe it'd be a good idea to add some in them too.

Sure, perhaps you can take a dig at Oauth.

@sansmoraxz
Copy link
Contributor Author

Quick update: I have implemented the core code changes.

I have a question regarding the unit tests, viz for checking lag? How are we going to approach it?

Can we use embeded docker container for this or is it gonna break something? For example: https://github.com/orlangure/gnomock

@zroubalik
Copy link
Member

@sansmoraxz I think we can give it a try, as discussed on Slack. Btw have you changed the scaler completely? I think that the safest option for us is to provide an alternative implementation and don't drop Sarama implementation (at least initially).

@sansmoraxz
Copy link
Contributor Author

Btw @sansmoraxz I think we can give it a try, as discussed on Slack. Btw have you changed the scaler completely? I think that the safest option for us is to provide an alternative implementation and don't drop Sarama implementation (at least initially).

Oops that I did. Need to rollback those. Yes it makes sense, to keep them seperate.
Should I implement this as a seperate scaler?

@dttung2905
Copy link
Contributor

Should we create a separate scaler i.e kafka scaler v2 for example? Then we can slowly deprecate the current one once the new one is stable

@stale
Copy link

stale bot commented Sep 4, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Sep 4, 2023
@sansmoraxz
Copy link
Contributor Author

/unstale

@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Sep 4, 2023
@github-project-automation github-project-automation bot moved this from To Do to Ready To Ship in Roadmap - KEDA Core Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request All issues for new features that have not been committed to needs-discussion
Projects
Archived in project
4 participants