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 for automatic navigation through the pages using regex in RabbitMQ Scaler #2087

Closed
wants to merge 5 commits into from

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Sep 1, 2021

This PR adds support for automatic navigation through the pages is scenarios when you are using regex with big amount of queues which match the regex.
With this, the total amount of queues is not a limit because now KEDA can recover any amount of queues using pagination
In order to expose information about if it's an intensive process, the total amount of queues is printed to the log (only the regex and the total amount of matching queues)

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated

Fixes #2068

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>
@coderanger
Copy link
Contributor

What's the benefit of this over just not using pagination? I'm worried this means data may be out of sync between pages.

@JorTurFer
Copy link
Member Author

What's the benefit of this over just not using pagination? I'm worried this means data may be out of sync between pages.

Hi @coderanger ,
It's a good question, as I explained in the issue, the monitor plugin has some limits about the amount of queues that you can recover at once. For example, CloudAMQP only allows a maximum page of 500 queues and the default amount of queues in the response is 100.
We have detected in our heaviest case (the regex matches with 723 queues) that we are missing information because of those limits (in KEDA all works, but we are processing only a subset of the total queues).
Recover all the pages should be a fast process, so I guess that the sync between the pages wouldn't be a problem.
I have another idea about this, and it is to make it user selectable, I mean, I can add a parameter to explicitly set if you want pagination, or you are comfortable without pagination.
In both cases (with user election or without it), I will add a note in the docs to explain clearly that in cases with a lot of pages, the data between pages could be not sync and in case of total sync requirement, the regex should match fewer queues.

The problem (in our use case, and probably in others) is that splitting the regex we can't do operations like sum or avg using all the queues, and we would like to take in consideration all the queues in the scaling operations.

wdyt?

@zroubalik
Copy link
Member

I keep this one on @coderanger

@JorTurFer
Copy link
Member Author

I keep this one on @coderanger

Hey @zroubalik , sorry, do you think that avoiding pagination is better? Shall we continue without them? Should we wait until @coderanger decission?
I can't get the exact meaning of your comment (sorry)

@zroubalik
Copy link
Member

I keep this one on @coderanger

Hey @zroubalik , sorry, do you think that avoiding pagination is better? Shall we continue without them? Should we wait until @coderanger decission?
I can't get the exact meaning of your comment (sorry)

I am not RabbitMQ expert, but I trust @coderanger, so I'd wait for his response :)

@JorTurFer
Copy link
Member Author

Now I got it!! :) (Thanks @zroubalik)
Yes, for sure. @coderanger is right in his point, I totally agree.
Let's wait until he takes a look at my latest proposal and shares his thoughtsWe don't have to rush, knowing this limit we are splitting the regexes.
The only think that I'd like to add if we don't support the navigation, it's to raise an error if the regex matches more queues those are returned (to clarify the error in that case)

@JorTurFer
Copy link
Member Author

Now I got it!! :) (Thanks @zroubalik) Yes, for sure. @coderanger is right in his point, I totally agree. Let's wait until he takes a look at my latest proposal and shares his thoughtsWe don't have to rush, knowing this limit we are splitting the regexes. The only think that I'd like to add if we don't support the navigation, it's to raise an error if the regex matches more queues those are returned (to clarify the error in that case)

Any update @coderanger ?

@coderanger
Copy link
Contributor

So my concern here is still that pagination in rmq-admin is not consistent, there's no way to be sure you won't be double counting or missing some data. I think this is a sign that you are using KEDA incorrectly, not something we should fix. A single scaler watching no more than 500 queues seems like a perfectly reasonable hard limit so we can ensure a consistent UX.

@coderanger
Copy link
Contributor

At a minimum this shouldn't be reducing the page size, since the core problem is we want to span page boundaries as rarely as possible.

@JorTurFer
Copy link
Member Author

JorTurFer commented Oct 5, 2021

What do you think about at least allow setting the page size? It could be enough for the majority of the cases, I agree with that our case is a specific case, and we should reduce the total amount of queues.
Maybe I can add an extra parameter to set the page size in case of using regex (default value is 100, not 500)

@coderanger
Copy link
Contributor

Yeah, a parameter to increase the page size sounds like a better fix. Puts more load on Rabbit but it ensures the data is correct :)

@JorTurFer
Copy link
Member Author

Nice!
I will close this PR because it's easier if I do it from main than update this PR. (We introduced a "security feature" which raises and error in case of the getting amount is lower than the regex matching count, so continue from that will be better)
I will refer this conversation in the new PR :)

@JorTurFer JorTurFer closed this Oct 5, 2021
@JorTurFer JorTurFer deleted the pagination branch November 5, 2021 17:44
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.

Add support to browse over the pages using regex in RabbitMQ Scaler
3 participants