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

feat: Support regex usage in Azure Service Bus scaler. #3607

Merged
merged 7 commits into from
Sep 30, 2022

Conversation

v-shenoy
Copy link
Contributor

@v-shenoy v-shenoy commented Aug 28, 2022

I chose regex instead of supporting a list in queueName / subscriptionName parameters. Reason for this being, if the number of entities is small enough to write in a list, the user might as well use multiple triggers within the ScaledObject. While regex will allow users to specify a large number of entities easily.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated and is aligned with our changelog requirements

Fixes #1624
Fixes #2920
Relates to kedacore/keda-docs#917

@v-shenoy v-shenoy requested a review from a team as a code owner August 28, 2022 17:46
@v-shenoy v-shenoy changed the title Feat/service bus feat: Support regex usage in Azure Service Bus scaler. Aug 28, 2022
@tomkerkhove
Copy link
Member

Reason for this being, if the list of entities is small enough to write in a list, the user might as well use multiple triggers within the ScaledObject

That's true but as soon as we support trigger ordering for evaluation or related cross-trigger features this goes away.

I do agree regex has its place, but maybe we need both? (list + regex?)

What are your thoughts @zroubalik @JorTurFer?

I personally wouldn't like to use regex as a list is simpler but see value in both.

@JorTurFer
Copy link
Member

I do agree regex has its place, but maybe we need both? (list + regex?)

For me, regex is totally enough, it's the approach we have in RabbitMQ, but I'm not against supporting both

@tomkerkhove
Copy link
Member

This is another one of those where I'm being a pain to try to increase the UX for non deep-experts

@v-shenoy
Copy link
Contributor Author

v-shenoy commented Aug 29, 2022

I feel having multiple parameters to do the same thing makes it more confusing for users. The regex approach also maintains parity with RabbitMQ.

@v-shenoy
Copy link
Contributor Author

v-shenoy commented Sep 1, 2022

Any more comments here @JorTurFer @tomkerkhove?

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Should we add any e2e test for this?

@v-shenoy
Copy link
Contributor Author

v-shenoy commented Sep 2, 2022

Looking good. Should we add any e2e test for this?

I'll add it. Should we cover for both queue and subscriber cases? (I don't mind adding them, I am just thinking about how the test suite size).

@JorTurFer
Copy link
Member

I'll add it. Should we cover for both queue and subscriber cases? (I don't mind adding them, I am just thinking about how the test suite size).

If possible, I'd test both, but IDK how complicated it is. Maybe if we add more test to azure, it's time also to create a folder to group all azure e2e test like we do with rabbit or redis

@v-shenoy
Copy link
Contributor Author

v-shenoy commented Sep 6, 2022

/run-e2e azure
Update: You can check the progress here

@v-shenoy
Copy link
Contributor Author

v-shenoy commented Sep 6, 2022

I'll add it. Should we cover for both queue and subscriber cases? (I don't mind adding them, I am just thinking about how the test suite size).

If possible, I'd test both, but IDK how complicated it is. Maybe if we add more test to azure, it's time also to create a folder to group all azure e2e test like we do with rabbit or redis

PTAL @JorTurFer

@tomkerkhove
Copy link
Member

What is the operation for? Not sure I get the usecase for it

@v-shenoy
Copy link
Contributor Author

What is the operation for? Not sure I get the usecase for it

Given that multiple queues / subscriptions can match the regex, we need an aggregation operation to convert the message counts into a single metric.

@JorTurFer
Copy link
Member

let's wait till the SDK update is merged (sorry for the extra work :( )

@v-shenoy
Copy link
Contributor Author

v-shenoy commented Sep 29, 2022

Updated after the SDK change. PTAL @JorTurFer

@v-shenoy
Copy link
Contributor Author

v-shenoy commented Sep 29, 2022

/run-e2e azure
Update: You can check the progress here

@v-shenoy
Copy link
Contributor Author

v-shenoy commented Sep 30, 2022

/run-e2e azure
Update: You can check the progress here

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@JorTurFer JorTurFer merged commit 344e468 into kedacore:main Sep 30, 2022
@v-shenoy v-shenoy deleted the feat/service-bus branch September 30, 2022 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants