keda icon indicating copy to clipboard operation
keda copied to clipboard

Add dapr checkpointing strategy for Azure Event Hubs scaler

Open brainslush opened this issue 2 years ago • 10 comments

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

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

Fixes #

Relates to https://github.com/kedacore/keda/issues/3141 Relates to https://github.com/kedacore/keda-docs/pull/775

brainslush avatar Jun 07 '22 18:06 brainslush

I already tested in our development infrastructure. It works.

brainslush avatar Jun 13 '22 08:06 brainslush

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.

brainslush avatar Jun 13 '22 17:06 brainslush

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

tomkerkhove avatar Jul 11 '22 05:07 tomkerkhove

@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.

brainslush avatar Jul 11 '22 22:07 brainslush

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!

tomkerkhove avatar Jul 14 '22 08:07 tomkerkhove

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.

brainslush avatar Jul 14 '22 13:07 brainslush

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

tomkerkhove avatar Jul 14 '22 13:07 tomkerkhove

Was that already done for Azure Event Hubs?

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

JorTurFer avatar Jul 14 '22 15:07 JorTurFer

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

zroubalik avatar Aug 04 '22 13:08 zroubalik

@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.

brainslush avatar Aug 10 '22 08:08 brainslush

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

JorTurFer avatar Oct 03 '22 13:10 JorTurFer

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

brainslush avatar Oct 05 '22 08:10 brainslush

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

zroubalik avatar Nov 04 '22 14:11 zroubalik

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

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

JorTurFer avatar Nov 04 '22 16:11 JorTurFer

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 (https://github.com/dapr/components-contrib/pull/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 avatar Nov 09 '22 08:11 christle

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

zroubalik avatar Nov 28 '22 10:11 zroubalik

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

christle avatar Nov 28 '22 12:11 christle

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

thanks a lot!

zroubalik avatar Nov 28 '22 12:11 zroubalik

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

brainslush avatar Nov 29 '22 10:11 brainslush

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

tomkerkhove avatar Dec 02 '22 08:12 tomkerkhove

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

christle avatar Dec 02 '22 09:12 christle

Thank you!

tomkerkhove avatar Dec 02 '22 10:12 tomkerkhove

Can we close this?

JorTurFer avatar Dec 06 '22 15:12 JorTurFer

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

tomkerkhove avatar Dec 06 '22 15:12 tomkerkhove