-
Notifications
You must be signed in to change notification settings - Fork 990
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
Azure.Messaging.EventHubs Checkpointing Implementation Changed as of v5.11.0 Causing Incorrect Behavior In Event Hub scaler #5574
Comments
Hello @windy1 , After checking the code, it looks that we still use Are you willing to open a PR including another check for the sequence? I guess that we cannot get rid of the offset field as it's used by Az functions AFAIK, but we should check if offset AND sequence are empty before returning an error |
Hi @JorTurFer I decided I would take a look, but I haven't been able to build the dev container successfully on my machine:
OS: macOS Sonoma 14.3.1 |
Nice catch! I've drafted a PR with a fix for devcontainers image. Could you apply that change? Basically you have to remove the line |
Regarding this bit, I was toying around with this today and I eventually came to the conclusion that checking if the offset, or the sequence number, is wholly redundant; at least in a dotnet / Azure SDK context. Unless I am misunderstanding here, there shouldn't ever exist checkpoints where the sequence number is empty, and as the code is written right now, this would fail anyway as it is expected to be an integer. Checkpoints for partitions that have never been checkpointed would simply not exist, thus; initialized checkpoints will always have a sequence number. It's very possible I am missing information about the checkpointing implementations in other contexts, such as Azure Functions, as you mentioned, but as far as I can tell, this is an impossible scenario if you are creating checkpoints through the Azure SDK. |
Yeah, that's the point I meant. Sequence number is the used property for all the calculations, but although the offset isn't used anywhere for the calculations, there are some if statements where the offset is used instead of the sequence number. We have to get rid of the offset usage for is statements in favor of sequence number |
What I'm saying is that I think the section you linked before can be removed completely and not replaced by some alternative method. |
I get your point, and I think you're right xD |
Report
Related: Azure/azure-sdk-for-net#42409
As of
Azure.Messaging.EventHubs v5.11.0
, the format of checkpoints written to blob storage has changed. Notably, the value ofoffset
isnull
. In our case, this caused KEDA to over-scale a service. Downgrading the SDK to v5.10.0 resolved the issue.In response to my original issue, it was asserted by a Microsoft contributor that this change is intentional and that the implementation of checkpoints are not to be relied upon.
Please refer to the original issue for more in-depth details and reproduction steps.
Expected Behavior
I expected KEDA to scale my service appropriately.
Actual Behavior
KEDA over-scaled the service until the Azure SDK was downgraded and the old checkpoint format restored.
Steps to Reproduce the Problem
Logs from KEDA operator
No response
KEDA Version
2.12.1
Kubernetes Version
None
Platform
Microsoft Azure
Scaler Details
Azure Event Hubs
Anything else?
No response
The text was updated successfully, but these errors were encountered: