-
Notifications
You must be signed in to change notification settings - Fork 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
Update GetAzureQueueLength in azure queue scaler, to support different queue length strategies. #5875
base: main
Are you sure you want to change the base?
Conversation
}) | ||
|
||
t.Run("Invalid base64 connection string", func(t *testing.T) { | ||
length, err := GetAzureQueueLength(context.TODO(), kedav1alpha1.AuthPodIdentity{}, "DefaultEndpointsProtocol=https;AccountName=name;AccountKey=key==;EndpointSuffix=core.windows.net", "queueName", "", "", "") |
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.
t.Error("Expected error to contain base64 error message, but got", err.Error()) | ||
} | ||
t.Run("Empty connection string", func(t *testing.T) { | ||
length, err := GetAzureQueueLength(context.TODO(), kedav1alpha1.AuthPodIdentity{}, "", "queueName", "", "", "") |
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.
d30026d
to
9b5fa4a
Compare
Semgrep found 2 Consider to use well-defined context Ignore this finding from context-todo. |
0f64726
to
a95588d
Compare
pkg/scalers/azure/azure_queue.go
Outdated
return int64(props.ApproximateMessagesCount()), nil | ||
} | ||
|
||
// Default strategy (visible + invisible messages) |
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.
As per kedacore/keda-docs#1406 (comment) I don't think default is a good name nor do we even use it here?
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, I have renamed it to 'all'.
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! Could you replace the magic string with a constant?
Thanks, hope the new commit makes it better. |
@leodip Can you fix merge conflicts please? |
bf331d5
to
e2421c0
Compare
…queueLengthStrategy Signed-off-by: Leonardo D'Ippolito <contact@leodip.com>
e2421c0
to
6bd92a1
Compare
@tomkerkhove I've squashed and rebased - conflicts should be solved now. |
This PR is an attempt to solve #4478
It adds a new configuration option
queueLengthStrategy
to the Azure Storage Queue scaler, where you can set the value ofdefault
orvisibleonly
.default
: Considers both visible and invisible messages.visibleonly
: Uses Peek to count only visible messages. If the count of visible messages is 32 or higher, it falls back to the default strategy, counting both visible and invisible messages.visibleonly
will, in practice, use the previous behaviour, as before #4003 was merged.Docs PR: kedacore/keda-docs#1406.
I've created this change to the best of my ability. However, it hasn't been tested. I would appreciate if someone could help with testing, or giving directions on how to test.
Thanks!