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

Allow providing separate host/port parameters for Redis scaler #694

Closed
3 tasks
marchmallow opened this issue Mar 20, 2020 · 12 comments
Closed
3 tasks

Allow providing separate host/port parameters for Redis scaler #694

marchmallow opened this issue Mar 20, 2020 · 12 comments
Labels
feature All issues for new features that have been committed to feature-request All issues for new features that have not been committed to needs-discussion scaler-redis-list

Comments

@marchmallow
Copy link
Contributor

Use-Case

The config for the Redis Lists scaler requires the following format for the host/port:

address: REDIS_HOST # Required host:port format

In our deployments we have separate env vars for hosts and ports, so would like to have an alternative to provide this information in separate parameters, similarly to how it is allowed for PostgreSQL and MySQL scalers.

Specification

  • Keep address param working as is
  • Add host and port parameters (look at implementation of MySQL/PostgreSQL as example)
  • Add config check and log an error if both address and host/port provided
@marchmallow marchmallow added feature-request All issues for new features that have not been committed to needs-discussion labels Mar 20, 2020
@marchmallow
Copy link
Contributor Author

@inuyasha82 is working on a prototype of this, we would like to know if you folks agree with adding this and any suggestion welcome. Thanks!

@tomkerkhove tomkerkhove added feature All issues for new features that have been committed to scaler-redis-list labels Mar 20, 2020
@tomkerkhove
Copy link
Member

That's a good point actually - I'm not sure if I'd bite the bullet and remove address or stick with both scenarios.

Thoughts by others?

@inuyasha82
Copy link
Contributor

@tomkerkhove
i think one problem is that scalers are not really standardized, so every scaler has a different behaviour.

i.e some like the redis scaler supporot only one format, usually host:port with different naming (redis has address, others have host, etc).

some like postgresql scaler have support for both format, host, port, and host:port but in this case is called "connection".

maybe some scalers doesn't support the host:port format.

@tomkerkhove
Copy link
Member

tomkerkhove commented Mar 20, 2020

@zroubalik @jeffhollan What do you think about this? Should we take this standardization into account for 2.0?

@zroubalik
Copy link
Member

I think we should, but I currently don't know all details about all scalers to make an informed decision. Therefore I don't know whether standardization is a good approach or not -> Do all scalers have similar needs/fields/paramteres to follow standardized format (given the nature of the scalers source, ie. queue/event/...)? Do we want to bring some partial mandatory format? Won't it bring unnecessary mess? We might need an analysis across all scalers, what parameters they are using and which way to see whether there is possibility to create some standard format.

@marchmallow
Copy link
Contributor Author

From looking at a number of the scalers which have proper docs linked at https://keda.sh/ I found one at least does not need this type of information: https://keda.sh/scalers/gcp-pub-sub/ but many others do and they are far from consistent. Most seem to only accept a single param that should contain host:port info, and the name of the parameter varies a lot, some examples: serverAddress, natsServerMonitoringEndpoint, connection, address

@marchmallow
Copy link
Contributor Author

I think it would make it simpler to learn how to work with different queues if this was a bit more standard.

@tomkerkhove
Copy link
Member

What I'm reading is that:

  1. We should not enforce mandatory fields
  2. Create a list of commonly used parameters and assign a default name
    • Example - Connection strings use a variety of names, but we should standardize on connectionString or connection
  3. We need to do the same for addresses in format of host:port, but provide alternative names for just host & port as well.
  4. In case of 3, both approaches are ok and we should standardize on which approach (full or seperate) to favorite over the other

Can everybody agree with that? If so, I'll create a dedicated issue for that.

In terms of #694 itself, totally agree that we should add it but prefer address indeed

@inuyasha82
Copy link
Contributor

@tomkerkhove yeah, i agree with your conclusions, as per 1 when you talk about mandatory fields, are u referring to some fields that should be commond in all scalers?
Cause think that every scaler needs some mandatory fields in order to establish the connection, but of course every queue needs different mandatory fields.

  1. Yeah i think it could be a good idea, makes things easier
    1. yeah definetely!

@inuyasha82 inuyasha82 mentioned this issue Mar 31, 2020
2 tasks
@inuyasha82
Copy link
Contributor

Hi everyone, just to let u know that i created PR #710 to implement this request.

@tomkerkhove
Copy link
Member

Thanks @inuyasha82!

See #712 for standardization!

@marchmallow
Copy link
Contributor Author

I think we can close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature All issues for new features that have been committed to feature-request All issues for new features that have not been committed to needs-discussion scaler-redis-list
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants