-
Notifications
You must be signed in to change notification settings - Fork 1.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
Cache metric names provided by KEDA Metrics Server #2279
Conversation
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
/run-e2e |
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
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.
LGTM!
Should we add a note in the docs explaining this? |
This change is just introducing a caching of the metric names that are provided by KEDA Metrics Server, not the actual metrics (values). It is internal change, so I am not sure that it is worth documenting. But I am open to do that if we all agree. |
I don't have a strong opinion about it, I was just sharing my thoughts :) |
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.
Looks good to me overall, just a few micro-optimization thoughts because I'm a terrible person :)
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
/run-e2e |
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
/run-e2e |
@ahmelsayed PTAL |
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.
@zroubalik LGTM. do you think it's worth wrapping the slice pointer and its lock into a separate struct? might make for easier-to-follow code later on, and reduce the likelihood of concurrency bugs down the line. I'd be happy to do that in a follow-up if you like.
@arschles I was thinking about similar approach, but not sure if it doesn't introduce additional complexity. But I am no strongly opposed to this. |
Signed-off-by: Zbynek Roubalik zroubali@redhat.com
Cache metrics provided by KEDA Metrics Server instead of requesting list of all ScaledObjects in the cluster, this should reduce the load on k8s server and Metrics Server should respond much faster.
Checklist
Fixes #2156
Potential fix for: #1937