Skip to content

add/remove shards in redis ring #2077

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.

Sign up for GitHub

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

Jump to bottom
Closed
szuecs opened this issue Apr 26, 2022 · 3 comments
Closed

add/remove shards in redis ring #2077

szuecs opened this issue Apr 26, 2022 · 3 comments
Labels
release-blocker

Comments

@szuecs
Copy link
Contributor

szuecs commented Apr 26, 2022 •
edited
Loading

We use redis ring client to shard access to redis for our rate limit infrastructure in Kubernetes https://opensource.zalando.com/skipper/tutorials/ratelimit/#redis-based-cluster-ratelimits.
I would like to add and remove shards on demand while Kubernetes is scaling-out redis instances.
I tried to implement it by closing and recreating the redis ring, but I think it would be better (less locks required) to trigger it via a library call.

One idea I had was to have a func() []string that is called every configurable time.Duration with a time.Ticker to set the Members and propagate these into the library ringShards. Or we could also do the triggering ourselves and the library just provides ReconfigureShards(shards []string).
Do you have a better idea how to make this happen?

I am willing to create a PR if it makes sense for you.
@vmihailenco
Copy link
Collaborator

@szuecs I think that providing API like ring.SetAddrs(addrs map[string]string) is a better way forward and I would gladly review such a PR 👍

@szuecs szuecs mentioned this issue May 25, 2022
Closed
@szuecs
Copy link
Contributor Author

szuecs commented Aug 1, 2022

@vmihailenco can you check the updated PR?
Thanks!

@szuecs
Copy link
Contributor Author

szuecs commented Oct 6, 2022

I think we can close this one

@szuecs szuecs closed this as completed Oct 6, 2022
AlexanderYastrebov added a commit to AlexanderYastrebov/go-redis that referenced this issue Nov 14, 2022
@AlexanderYastrebov
ring: reduce SetAddrs shards lock contention
31e835e 
Introduces a new lock to make `SetAddrs` calls exclusive.
This enables removal of `ringSharding` field locking for the duration of
potentially long `newRingShards` call.
Closes newly created shards if `ringSharding` was closed during `SetAddrs`.

`TestRingSetAddrsContention` observes number of pings increased from ~200 to ~30_000.

See related discussion redis#2190 (comment)

Updates redis#2077
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Nov 15, 2022
Merged
AlexanderYastrebov added a commit to AlexanderYastrebov/go-redis that referenced this issue Nov 21, 2022
@AlexanderYastrebov
test: add TestRingSetAddrsContention
a31c1d6 
See redis#2190 (comment)

Updates redis#2077
AlexanderYastrebov added a commit to AlexanderYastrebov/go-redis that referenced this issue Nov 21, 2022
@AlexanderYastrebov
fix: reduce SetAddrs shards lock contention
eb193eb 
Introduces a new lock to make `SetAddrs` calls exclusive.
This allows release of shards lock for the duration of potentially long `newRingShards` call.

`TestRingSetAddrsContention` observes number of pings increased from <1000 to ~40_000.

Updates redis#2077
AlexanderYastrebov added a commit to AlexanderYastrebov/go-redis that referenced this issue Nov 21, 2022
@AlexanderYastrebov
fix: reduce SetAddrs shards lock contention
c117bb2 
Introduces a new lock to make `SetAddrs` calls exclusive.
This allows release of shards lock for the duration of potentially long `newRingShards` call.

`TestRingSetAddrsContention` observes number of pings increased from <1000 to ~40_000.

See redis#2190 (comment)

Updates redis#2077
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Nov 21, 2022
Merged
AlexanderYastrebov added a commit to AlexanderYastrebov/go-redis that referenced this issue Nov 22, 2022
@AlexanderYastrebov
fix: reduce SetAddrs shards lock contention
6c05a9f 
Introduces a new lock to make `SetAddrs` calls exclusive.
This allows release of shards lock for the duration of potentially long `newRingShards` call.

`TestRingSetAddrsContention` observes number of pings increased from <1000 to ~40_000.

See redis#2190 (comment)

Updates redis#2077
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Nov 28, 2022
@AlexanderYastrebov
net: update go-redis client
5332abd 
#2009 intruduced go-redis
client fork that contains neccesary performance improvements for
`ring.SetAddrs` API.

Since then upstream go-redis client was improved so this change
removes the fork.

See
* redis/go-redis#2077
* #2009

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Nov 28, 2022
@AlexanderYastrebov
net: update go-redis client
b3dad7b 
#2009 intruduced go-redis
client fork that contains implementation of `ring.SetAddrs` API.

Implementation of this API with improvements was later added to
the upstream so this change removes the fork.

See
* redis/go-redis#2077
* #2009

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Nov 28, 2022
Merged
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Feb 8, 2023
@AlexanderYastrebov
net: update go-redis client
023901a 
#2009 intruduced go-redis
client fork that contains implementation of `ring.SetAddrs` API.

Implementation of this API with improvements was later added to
the upstream so this change removes the fork.

See
* redis/go-redis#2077
* #2009

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Feb 8, 2023
@AlexanderYastrebov
net: update go-redis client
9c11632 
#2009 intruduced go-redis
client fork that contains implementation of `ring.SetAddrs` API.

Implementation of this API with improvements was later added to
the upstream so this change removes the fork.

See
* redis/go-redis#2077
* #2009

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Apr 6, 2023
@AlexanderYastrebov
net: update go-redis client
7705fa0 
#2009 intruduced go-redis
client fork that contains implementation of `ring.SetAddrs` API.

Implementation of this API with improvements was later added to
the upstream so this change removes the fork.

See
* redis/go-redis#2077
* #2009

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue May 15, 2023
@AlexanderYastrebov
net: update go-redis client
69a0949 
#2009 intruduced go-redis
client fork that contains implementation of `ring.SetAddrs` API.

Implementation of this API with improvements was later added to
the upstream so this change removes the fork.

See
* redis/go-redis#2077
* #2009

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue May 15, 2023
@AlexanderYastrebov
net: update go-redis client (#2159)
cffe139 
#2009 intruduced go-redis
client fork that contains implementation of `ring.SetAddrs` API.

Implementation of this API with improvements was later added to
the upstream so this change removes the fork.

See
* redis/go-redis#2077
* #2009

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
release-blocker
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feature: dynamically add and remove shards by the ring client szuecs/redis
2 participants
@szuecs @vmihailenco