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

Prefix for 'goSDK' checkpoint strategy for Azure Event Hub scalter #3022

Closed
brainslush opened this issue May 8, 2022 · 19 comments
Closed

Prefix for 'goSDK' checkpoint strategy for Azure Event Hub scalter #3022

brainslush opened this issue May 8, 2022 · 19 comments
Labels
enhancement New feature or request feature-request All issues for new features that have not been committed to help wanted Looking for support from community needs-discussion

Comments

@brainslush
Copy link

Proposal

I suggest to add the possibility to define a prefix for the 'goSDK' checkpoint strategy in the trigger specification of the Azure Event Hub scaler.

e.g.

triggers:
- type: azure-eventhub
  metadata:
    connectionFromEnv: EVENTHUB_CONNECTIONSTRING_ENV_NAME
    storageConnectionFromEnv: STORAGE_CONNECTIONSTRING_ENV_NAME
    consumerGroup: $Default
    unprocessedEventThreshold: '64'
    blobContainer: 'name_of_container'
    goSdkPrefix: 'someDir/dapr-dapreventhubname-daprconsumergroup-'
    checkpointStrategy: goSdk

The search path for the checkpoint file would look like:
https://myaccount.blob.core.windows.net/name_of_container/someDir/dapr-dapreventhubname-daprconsumergroup-0

Use-Case

Allows usage of Keda together with DAPR and Azure Event Hub.
Dapr creates files on the blob storage which are named
{blobContainer}/dapr-{topic}-{consumerGroup}-{partitionId} .
Keda on the other hand looks for
{blobContainer}/{partiotionId}.

Anything else?

Even though I don't know go yet I would try myself on a PR.

@brainslush brainslush added feature-request All issues for new features that have not been committed to needs-discussion labels May 8, 2022
@tomkerkhove tomkerkhove added the enhancement New feature or request label May 9, 2022
@tomkerkhove
Copy link
Member

There are two options here, we can do either or both:

  1. Support a prefix/format, potentially requiring value injection as shown above
  2. Support dapr checkpointing, but then this same problem might come up later on with another product

Thoughts @kedacore/keda-core-contributors?

@brainslush
Copy link
Author

I would go with the value injection approach as it leaves room for customization.

@tomkerkhove
Copy link
Member

That is a good option, but doing both might be good as well given every Dapr end-user would otherwise have to do this customization which is less trivial from a UX standpoint.

@brainslush
Copy link
Author

A suggestion:

triggers:
- type: azure-eventhub
  metadata:
    connectionFromEnv: EVENTHUB_CONNECTIONSTRING_ENV_NAME
    storageConnectionFromEnv: STORAGE_CONNECTIONSTRING_ENV_NAME
    consumerGroup: $Default
    unprocessedEventThreshold: '64'
    blobContainer: 'name_of_container'
    goSdkTemplate: 'dapr'
    checkpointStrategy: goSdk

-> {blobContainer}/dapr-{topic}-{consumerGroup}-{partitionId}

triggers:
- type: azure-eventhub
  metadata:
    connectionFromEnv: EVENTHUB_CONNECTIONSTRING_ENV_NAME
    storageConnectionFromEnv: STORAGE_CONNECTIONSTRING_ENV_NAME
    consumerGroup: $Default
    unprocessedEventThreshold: '64'
    blobContainer: 'name_of_container'
    goSdkTemplate: 'prefix'
    goSdkPrefix: 'someDir/somePrefix-'
    checkpointStrategy: goSdk

-> {blobContainer}/someDir/somePrefix-{partitionId}

@tomkerkhove
Copy link
Member

If we go that route, I wouldn't use goSdk* but something more generic in case we want to do this for other checkpoint strategies as well.

@brainslush
Copy link
Author

How about checkpointStrategyTemplate and chekpointPrefix ?

@tomkerkhove
Copy link
Member

Fine by me. Thoughts @zroubalik & @JorTurFer? I'd add a Dapr strategy as well though.

@JorTurFer
Copy link
Member

I agree with @tomkerkhove adding a Dapr strategy because it has value itself and for future product integrations, we can support also the checkpointStrategyTemplate/chekpointPrefix

@tomkerkhove
Copy link
Member

Are you willing to still contribute this @brainslush?

@brainslush
Copy link
Author

I'll try. Haven't done coding in Go before but when I read the code I got the impression that this shouldn't be difficult.

@JorTurFer JorTurFer added the help wanted Looking for support from community label May 23, 2022
@christle
Copy link
Contributor

Hi,

As i checked the dapr behavior with container apps and the eventhub scaler, a view month ago, it worked well with the goSdk. But the eventhub component was rewritten:

dapr/components-contrib@29e22a0#diff-3a38d7491c58638ad54630db869afd8fbd80db683f62dc62f18a9c5b214b0962

which is good, we are waiting for some changes too :-)

So, I'm with @tomkerkhove. from the end-user perspective dapr is more concrete and readable. For example, if you create a container app you don't want to configure things like a template path. This is an implementation detail.

I would focus on some concrete checkpoint strategy implementations and not a generic solution that lifts the complexity into the scaler configuration. Most of the eventhub processor implementations like in c#, Azure Functions, java, javascript, or python switched over to the metadata checkpoint strategy. So the gosdk variant is currently a very specific one.

just my opinion. I think If there is a need for a very generic solution, it could be added later. So i guess the dapr one is more important.

@brainslush, i started last year with the checkpoint strategy variant. If you need help or want that i jump in, feel free to ask!

@brainslush
Copy link
Author

brainslush commented May 31, 2022

I'm currently a bit busy but I still intend to do it.
@christle But, in case I get stuck I'll get back to you. I agree that implementing a specific strategy is the best solution. I had a chat with the dapr developers and they assured me that they intend to keep the path and behavior as it is right now.

@tomkerkhove
Copy link
Member

Created #3141 for Dapr checkpointing

@christle
Copy link
Contributor

christle commented Aug 6, 2022

FYI: I've created a PR on Dapr to bring that topic forward: dapr/components-contrib#1940

@tomkerkhove
Copy link
Member

Does that mean we will only support the new Dapr version or not? (just to verify)

@christle
Copy link
Contributor

christle commented Aug 9, 2022

For older versions, the goSdk Checkpoint Strategy works like before. But since January 2022, this strategy doesn't work anymore for pubsub because of the naming chance. But still for bindings.

With the small dapr change, for new dapr versions, a new checkpoint strategy "dapr" is needed. For older versions, the goSdk checkpoint strategy works well.

@tomkerkhove
Copy link
Member

Oh boy, this will be fun for support cases - Thanks for following up and let's make sure we have good docs on this :)

@JorTurFer
Copy link
Member

I think this is already done. We can reopen it if not

@tomkerkhove
Copy link
Member

Related: #3141

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature-request All issues for new features that have not been committed to help wanted Looking for support from community needs-discussion
Projects
Archived in project
Development

No branches or pull requests

4 participants