-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
language agnostic checkpointing for azure eventhub scaler #1621
Conversation
Thanks @christle for the contribution. My main concern is that this is a breaking change for eventhub scaler. @tomkerkhove would probably know better than me how we should handle such breaking changes. |
As I understand, the azure functions are currently the only checkpointing that works. This is the type AzureWebJob and for that, everything stays the same. No additional configuration is necessary either. All other frameworks are currently not executable. But it may be that I'm wrong. If there are currently still variants, they use probably this old Python checkpointing behavior. If want to support old Phyton checkpointing we could add a specific python checkpointer and set it as the default behavior if a container name is set. |
i think the point with the old python behavior is not bad. I could add it as an additional backward checkpointer. Then we make sure all stays the same. But if anybody will use checkpointing with die current python- dapr- java- eventhub framework and so on, then the checkpointType have to be set explcitly, in this case. |
That is not correct, actually. As per the docs:
We'll need to be backwards compatible with all of them. |
The current implementation in 2.1 is this: containerName == "" -> use Webjob Behavior (azure function) -> that means use "azure-webjobs-event" as containername currently my change is: what i can add to make it backwards compatible even for old sdks: i think then we have all former SDK's supported and additional the new checkpointing for java, python + the gosdk. And a open structure to support new checkpointer for more languages which special checkpoint behavior. But i think there will be more sdks that use it like the new Java and Python checkpointer. So i think the suggested default checkpointer support more than these both at the moment. |
69293dd
to
19c38ed
Compare
hi @ahmelsayed, i've added a new checkpointer for backward compatibility. Now there a 4 checkpointer: DefaultCheckpointer (Old Java SDK, Old Python SDK, C#, maybe more) AzureWebJobCheckpointer (Azure Functions) BlobMetadataCheckpointer (Current Java SDK, Current Python, maybe more) GoSdkCheckpointer (GoSdk and Dapr for example which use the GoSdk) new Checkpointer could be added at the same way. Only the implementation of this interface is required:
could review that? i would left the PR on DRAFT, because i've already tested all checkpointer on a real storage blob and integrationtests but i want to test it with a complete keda build inside our cluster. Then i can remove the draft, from my side. |
These are a lot of different types to checkpoint, which is fine for me! However, instead of being so implicit, I would introduce Wdyt @ahmelsayed @zroubalik? |
The logic itself and all looks great btw, thanks! |
i just renamed checkpointType to checkpointStrategy. The values are unchanged. Is this ok? |
Fine for me, what do you think @zroubalik @ahmelsayed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
i just removed the draft flag. Yesterday i've made a successful live test on our dev cluster. |
Would you mind:
|
132ff93
to
06ce04a
Compare
done. I move over to the docs repo |
the docs PR: kedacore/keda-docs#429 |
For JavaScript the Azure SDK for JS provides a BlobCheckpointStore class which implements the required read/writes to a durable store by using Azure Blob Storage in case you want to see if this implementation would also work for JS. |
i changed the code to lowercase for checkpointStrategy parameters, @zroubalik @nzthiago thanks, for your link! I've checked the implementation and it looks like the JS framework works with the "BlobMetadata" checkpointStrategy like Java and Python. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@tomkerkhove ok to merge this? |
Signed-off-by: Christian Leinweber <christian.leinweber@maibornwolff.de>
… scaler Signed-off-by: Christian Leinweber <christian.leinweber@maibornwolff.de>
Signed-off-by: Christian Leinweber <christian.leinweber@maibornwolff.de>
Signed-off-by: Christian Leinweber <christian.leinweber@maibornwolff.de>
Signed-off-by: Christian Leinweber <christian.leinweber@maibornwolff.de>
Signed-off-by: Christian Leinweber <christian.leinweber@maibornwolff.de>
…tion Signed-off-by: Christian Leinweber <christian.leinweber@maibornwolff.de>
fixed merge conflict on CHANGELOG.md |
this is a solution for a language agnostic checkpointing.
It comes with a new konfiguration for eventhub scaler: checkpointType.
Actually with 3 Types:
AzureWebJob: the is default if no blobcontainer is set. this is the current working behavior for azure functions
GoSdk: GoSdk has a very special storage path and a special checkpoint structure.
Default: Default is set if a blobcontainer is set. Default, because this path and checkpoint structure is used by java, python and maybe others. It stores the values as metadata on the checkpoint blob.
Fixes #769 #767 #741