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

add support for multiple redis list types in redis list scaler #1006

Closed
wants to merge 1 commit into from

Conversation

ilyamor
Copy link
Contributor

@ilyamor ilyamor commented Aug 18, 2020

added support for multiple redis "list" types scaling, including : zset,hash,set and list

Checklist

Fixes #

@tomkerkhove
Copy link
Member

  • Changelog has been updated - not sure what it this

This can be found on https://github.com/kedacore/keda/blob/master/CHANGELOG.md

Thanks for updating!

}

var cmd *redis.IntCmd
switch listType.Val() {
Copy link
Member

Choose a reason for hiding this comment

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

Do we just pull from the correct type or does the type have to be configured?

Copy link
Contributor

Choose a reason for hiding this comment

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

The way I'm reading the change, and I'm no redis expert btw, is that the particular list we're trying to scale on could have different types that require a different call on the client. I think the approach looks fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we just pull from the correct type or does the type have to be configured?

it may be configured by the user but we sill will have to do switch casing on it for different api call to redis, so this approach is more flexible to the user

Copy link
Member

Choose a reason for hiding this comment

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

If it's a type of list, then it's ok for me but if it's a different type of entity I would make it configurable on what people want to get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets call it collection instead of a list, and you have different type of collections(list, set, map, etc ..) that all have "name" and "length" and those are the only parameters i would want the user to configure


cmd := client.LLen(listName)
listType = client.Type(listName)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if list name is appropriate here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any other suggestions ?

@tomkerkhove
Copy link
Member

Docs are up on kedacore/keda-docs#231.

Just out of curiosity - Why can't we add a test for this?

@zroubalik
Copy link
Member

Could you please open this PR against v2 branch? We don't expect to bring any big features to v1.

ilyamor pushed a commit to ilyamor/keda that referenced this pull request Aug 19, 2020
@ilyamor ilyamor changed the base branch from master to v2 August 19, 2020 11:21
ilyamor pushed a commit to ilyamor/keda that referenced this pull request Aug 19, 2020
@ilyamor
Copy link
Contributor Author

ilyamor commented Aug 19, 2020

open new pr to v2
#1013

@ilyamor
Copy link
Contributor Author

ilyamor commented Aug 19, 2020

Docs are up on kedacore/keda-docs#231.

Just out of curiosity - Why can't we add a test for this?

the test will not cover any business logic only "testing the database"

@ilyamor ilyamor changed the base branch from v2 to master August 19, 2020 11:32
@ilyamor ilyamor closed this Aug 19, 2020
SpiritZhou pushed a commit to SpiritZhou/keda that referenced this pull request Jul 18, 2023
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.

4 participants