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

Adds support to use regex with HTTP protocol in RabbitMQ Scaler #1957

Merged
merged 12 commits into from
Jul 27, 2021
Merged

Adds support to use regex with HTTP protocol in RabbitMQ Scaler #1957

merged 12 commits into from
Jul 27, 2021

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Jul 13, 2021

Checklist

This PR adds support to use regex as a queue selector in RabbitMQ Scaller. With this changes, complex cases with more than 1 queue in the application topology are covered and can be scaled with RabbitMQ Scaler

Closes #743

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
@coderanger
Copy link
Contributor

Could you explain the use case for this? Looks like your aim is to scale something consuming from multiple queues?

@JorTurFer
Copy link
Member Author

Could you explain the use case for this? Looks like your aim is to scale something consuming from multiple queues?

Sure! For example, in our use case we have more than 1 queue (near of 70) that are consumed by one hub to do some stuffs. You can imagine it as a "central hub". In this case, you could need to scale in base of all the queues instaed 1.
RabbitMQ supports the use of regex when you are using the HTTP protocol, so I think it's not too strange case.

@coderanger
Copy link
Contributor

Yeah, the use case seems reasonable just wondering the best way to expose it. This hardwires taking the highest values out of any returned queue which I think should probably be fine? Might be some use cases around "mean value" but nothing super obvious comes to mind. And we should make sure that activating it in AMQP mode is a hard error so we don't get silent "it's just not working" behavior.

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 13, 2021

You are right, I will update the code to raise an explicit error in case of invalid configuration. Related with the max value or the mean value, I don't have any strong about this point, I did with max value because the rate can't be usable to get the mean value and the maximum yes.
What do you think if I add support only for message count (not for rate) and I support the operations sum, max y avg?
This can be well documented and raise an error in case of a wrong configuration to avoid silent errors. Probably it's more useful than my current design.

I don't have too much time so I will do little changes during the week. I will stay tuned to your comments :)

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
@tomkerkhove tomkerkhove marked this pull request as draft July 14, 2021 06:40
@tomkerkhove tomkerkhove changed the title Adds support to use regex with HTTP protocol in RabbitMQ Scaler (WIP) 🚧 Adds support to use regex with HTTP protocol in RabbitMQ Scaler Jul 14, 2021
JorTurFer and others added 7 commits July 14, 2021 20:35
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
@JorTurFer
Copy link
Member Author

Could you approve the CI workflow please? I think that is fixed (at least in my own repo)
https://github.com/JorTurFer/keda/actions/runs/1033020481

@JorTurFer JorTurFer marked this pull request as ready for review July 15, 2021 19:08
@JorTurFer JorTurFer changed the title 🚧 Adds support to use regex with HTTP protocol in RabbitMQ Scaler Adds support to use regex with HTTP protocol in RabbitMQ Scaler Jul 15, 2021
@rwkarg
Copy link
Contributor

rwkarg commented Jul 16, 2021

For multi-queue use cases, we add multiple triggers for each queue which will have the same behavior of scaling at the maximum queue size or publish rate, but it will expose each of the scaling metrics in the HPA (instead of the external metric server collapsing it to one metric for the HPA).

I don't know that the current setup would support querying the metrics by regex, reporting each of those metrics, then updating the HPA with a metric for each observed queue (or only when there's a change). This would give visibility through the HPA what metrics are being evaluated for scaling decisions.

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM

@zroubalik zroubalik added this to the v2.4.0 milestone Jul 21, 2021
Jorge Turrado added 3 commits July 23, 2021 00:33
Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
@JorTurFer JorTurFer requested a review from zroubalik July 22, 2021 22:50
@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 22, 2021

Hi @zroubalik
I have requested your review again because I have done some changes in the code to allow use regex with messageRate mode. Sorry for the inconvenient :(
I have just updated the documentation as well :)

Copy link
Member

@zroubalik zroubalik 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 thanks for the contribution!

@zroubalik zroubalik merged commit 50cffa1 into kedacore:main Jul 27, 2021
nilayasiktoprak pushed a commit to nilayasiktoprak/keda that referenced this pull request Oct 23, 2021
…core#1957)

Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
Signed-off-by: nilayasiktoprak <nilayasiktoprak@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can Rabbitmq scalar be used with regexes ?
5 participants