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

Add dapr checkpointing strategy for Azure Event Hubs scaler #3139

Closed
wants to merge 2 commits into from

Conversation

brainslush
Copy link

@brainslush brainslush commented Jun 7, 2022

This PR adds the dapr checkpointing strategy to the Azure Event Hubs scalar. The new checkpoint strategy is based on the existing goSdk checkpointing strategy but implements the dapr specific naming scheme for the offset file on the Azure Blob Storage. To use this strategy the field checkpointStrategy in the trigger specification needs to be set to the value dapr.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated and is aligned with our changelog requirements

Fixes #

Relates to #3141
Relates to kedacore/keda-docs#775

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK how it works, but from code pov looks good. Only one typo in the changelog
❤️ ❤️ ❤️

CHANGELOG.md Outdated Show resolved Hide resolved
@JorTurFer JorTurFer requested a review from a team June 8, 2022 20:01
@zroubalik zroubalik changed the title Add dapr checkpointing strategy for Azure Event Hubs scalar Add dapr checkpointing strategy for Azure Event Hubs scaler Jun 13, 2022
@brainslush
Copy link
Author

I already tested in our development infrastructure. It works.

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this could be covered by an e2e test?

@brainslush
Copy link
Author

I think it could be e2e testable.
But, this would definitely take some more time to implement, at least for me as I am still a novice with Go.
I can try to implement it but I probably would suggest to create a new issue for it. I'm pretty busy next few weeks so I don't know if I'll have the time in the very near future.

@tomkerkhove
Copy link
Member

@brainslush Any thought if we can close this PR soon? We are planning to release in 2 weeks.

@brainslush
Copy link
Author

brainslush commented Jul 11, 2022

@tomkerkhove
I have no concerns so far. I tested it for some time now in our development infrastructure w/o any issues. Do I still need to do something?

Concerning the e2e test, I currently don't have the time as I'll move in a month to another city but after the move I'll take a shot at it.

@tomkerkhove
Copy link
Member

No problem at all, but we'll hold off merging in that case until we have some e2e tests though.

Good luck with your move!

@brainslush
Copy link
Author

No problem at all, but we'll hold off merging in that case until we have some e2e tests though.

Good luck with your move!

I have seen that you are currently moving e2e to go. Was that already done for Azure Event Hubs?
When exactly do you plan to do the release? I have to see if I possibly can squeeze it in somewhen.

@tomkerkhove
Copy link
Member

We are shooting for the first week of August. I'll ask @JorTurFer about Event Hubs though, he should know

@JorTurFer
Copy link
Member

Was that already done for Azure Event Hubs?

not yet, you can do it if you want :)

@zroubalik
Copy link
Member

@brainslush any update on this please? We are going to ship a new release next week.

@brainslush
Copy link
Author

@zroubalik
I'm currently looking into how to do integration tests in golang. I'll do it. As I said I am currently moving to a new city, so free time is very sparse.

@JorTurFer
Copy link
Member

JorTurFer commented Oct 3, 2022

hey @brainslush
Today we have merged a PR with 2 new e2e test for Event Hub, one for blobMetadata strategy and another for goSdk strategy.
You can create a new test based on them but you should add also Dapr installation/removal as part of the e2e tests.
You have an example about how to install and uninstall with AWS Identity Components, but in this case the installation is not conditional, you should install and uninstall Dapr on every execution

@brainslush
Copy link
Author

@JorTurFer I'll have a look at it. Thanks

@zroubalik
Copy link
Member

@JorTurFer @brainslush what is the status of this PR, please?

@JorTurFer
Copy link
Member

@JorTurFer @brainslush what is the status of this PR, please?

I don't have more info about this, waiting e2e tests xD

@christle
Copy link
Contributor

christle commented Nov 9, 2022

If @brainslush needs support, I could jump in for that. (I created another duplicate PR because I missed this one). So if wanted, i could create a little e2e test.

I would be great if we could merge this for the next release because my dapr change is merged within dapr 1.9 (dapr/components-contrib#1940) I have consolidated the naming schema between pubsub and bindings.

Additionally, related to this pr i would add to the docs pr a version-info for the new dapr checkpointer.

so the new dapr checkpointer works for:
pubsub components: >= Dapr 1.6 (older versions need the GoSdk checkpointer)
binding components: >= Dapr 1.9 (older versions need the GoSdk checkpointer)

So we need for newer dapr versions, this new checkpointer. I can add this version Infos to the docs pr

@zroubalik
Copy link
Member

If @brainslush needs support, I could jump in for that. (I created another duplicate PR because I missed this one). So if wanted, i could create a little e2e test.

I would be great if we could merge this for the next release because my dapr change is merged within dapr 1.9 (dapr/components-contrib#1940) I have consolidated the naming schema between pubsub and bindings.

Additionally, related to this pr i would add to the docs pr a version-info for the new dapr checkpointer.

so the new dapr checkpointer works for: pubsub components: >= Dapr 1.6 (older versions need the GoSdk checkpointer) binding components: >= Dapr 1.9 (older versions need the GoSdk checkpointer)

So we need for newer dapr versions, this new checkpointer. I can add this version Infos to the docs pr

@christle sounds like a good plan, do you think you can follow up on the work by @brainslush? btw the next release is in ~2 weeks

@christle
Copy link
Contributor

Yes, I could create an e2e test this week and prepare a docs pr for 2.9

@zroubalik
Copy link
Member

Yes, I could create an e2e test this week and prepare a docs pr for 2.9

thanks a lot!

@brainslush
Copy link
Author

@christle
That would be great. I still haven't gotten around to implement the e2e-test.

@tomkerkhove
Copy link
Member

We are going to release KEDA v2.9 on Thursday. Do you think you can complete the open work by Wednesday @christle?

@christle
Copy link
Contributor

christle commented Dec 2, 2022

yes, I think so. I'm nearly ready with the e2e test. I found an easy way to integrate Dapr into the test setup. I will also open a PR for the test-tools repo

@tomkerkhove
Copy link
Member

Thank you!

@JorTurFer
Copy link
Member

Can we close this?

@tomkerkhove
Copy link
Member

Let's - Thanks for the contribution anyway @brainslush!

@tomkerkhove tomkerkhove closed this Dec 6, 2022
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.

5 participants