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

Solace Scaler: Enhancement to support a hostlist of Solace brokers #6541

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

dpochopsky
Copy link

@dpochopsky dpochopsky commented Feb 12, 2025

Provide a description of what has been changed

Checklist

  • [ x] When introducing a new scaler, I agree with the scaling governance policy
  • [ x] I have verified that my change is according to the deprecations & breaking changes policy
  • [ x] Tests have been added
  • [ x] Changelog has been updated and is aligned with our changelog requirements
  • [ x] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • [ x] A PR is opened to update the documentation on (repo) (if applicable)
  • [ x] Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #

Relates to #

@dpochopsky dpochopsky requested a review from a team as a code owner February 12, 2025 18:18
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.

Thanks for the contribution! A naive question, what's this for? I mean, which is the use case of multiple brokers?

Signed-off-by: David Pochopsky <[email protected]>
Signed-off-by: David Pochopsky <[email protected]>
Signed-off-by: David Pochopsky <[email protected]>
Signed-off-by: David Pochopsky <[email protected]>
@dpochopsky
Copy link
Author

dpochopsky commented Feb 12, 2025

Thanks for the contribution! A naive question, what's this for? I mean, which is the use case of multiple brokers?

The use case is for Solace deployments with Disaster Recovery (DR) enabled. Solace DR introduces an additional Solace Broker and operates in an active standby mode, so the scaler needs to query two different brokers with different endpoints, verify connectivity status as well as determine which of the brokers are active, once that is deteremined, the scaler operates as it did before.

@JorTurFer
Copy link
Member

Interesting point and rally nice addition! The only thing that I'm not sure is about querying all the brokers all the time (if the 1st one isn't the active, it'll be asked all the time). I think that we should just store which one is the active one and relaying of it. As KEDA will recreate the trigger on errors, if the active broker fails, the scaler will fail and KEDA will recreate it, refreshing the active broker

@dpochopsky
Copy link
Author

I initially planned on maintaining the brokers' state as you suggested, but I was told there isn't a way to maintain state. Despite that I still gave it a try and couldn't figure out how to do it.

@JorTurFer
Copy link
Member

I initially planned on maintaining the brokers' state as you suggested, but I was told there isn't a way to maintain state. Despite that I still gave it a try and couldn't figure out how to do it.

just store it as part of a scaler scoped variable

@dpochopsky
Copy link
Author

I initially planned on maintaining the brokers' state as you suggested, but I was told there isn't a way to maintain state. Despite that I still gave it a try and couldn't figure out how to do it.

just store it as part of a scaler scoped variable

ok I'll give that a try, thanks

@dpochopsky
Copy link
Author

I initially planned on maintaining the brokers' state as you suggested, but I was told there isn't a way to maintain state. Despite that I still gave it a try and couldn't figure out how to do it.

just store it as part of a scaler scoped variable

enhancement to track which of the hosts are active has been implemented as requested.

@JorTurFer
Copy link
Member

You're storing the current index, but you're not using it, so each metric request is querying both solace all the time. I meant to store the current broker to just call to that broker only once it's been detected

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.

Could you please open an issue to describe the intention behind this PR? Then link it here and in the Changelog.

Please also link here the related docs PR.

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.

3 participants