-
Notifications
You must be signed in to change notification settings - Fork 23.5k
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
Feature Request: Add ability to expire members of a set #135
Comments
Hello, unfortunately it is not in our plans! Sorry. The one sentence rationale is: too complex, too memory consuming, more CPU needed. |
This is not going to be added. There is a lot of extra memory involved, not to mention a lot of processing to make sure the elements would in fact be expired. The best way to implement this is to model this yourself, so you can stick to the bare minimum you need, while keeping all the flexibility to change things as you please. The best pattern to model this is, in my opinion, using sorted sets with a score equal to the timestamp the member should expire. You can use Good luck. |
+1 for sorted sets for this task. We use that for millions of integers in sorted set and it's ok |
+1 |
+1 would be a great feature (but obviously comes with a cost) |
+1 |
+1 It should be added. Memory is a give&take the user should consider. If we need the feature indeed, we will take the result of using more memory for sure. |
User seen per project are now stored in a zset so that the cache of a project can be invalidated. The implementation is based upon the recommendation/trick mentionned here [0] since it is not possible to set an expiration on a member of a set. No functionnal changes are expected from this contribution, the invalidation of the cache when a project has a status change will done in a future contribution. You can however check it works as expected by dropping the set of seen username per project and check that you get a cache miss: DEL apache_svn_project_set_<project_id> While this contribution does not add a new feature, the error introduced by commit 26cbef9 ("Can't locate object method 'log' via package 'Apache2::Log::Request'") is fixed. To test, you need to redeploy the Tuleap.pm file to /usr/share/perl5/vendor_perl/Apache/Tuleap.pm and restart Apache. This is part of story #12160: have a clean way to suspend a project [0] redis/redis#135 (comment) Change-Id: Ib056f7c3ca1c37bdf39de8c1106ff78e248c9970
What about tying a command to an EXPIRE? That way we can expire keys but trigger the deletion of a specific member of a set? |
+1 |
+1, Expiry time for Redis set members will be highly appreciated. |
This should be added. Obviously there is more memory and computation involved. But without this, everyone who needs this will need more memory and computation AND the developer time to implement the same sorted set solution. |
+1 It would be nice if this feature were added because then someone can decide whether or not to use it, it would probably end up being slower if a dev implemented this themselves anyways. |
This should be added. Obviously there is more memory and computation involved. But without this, everyone who needs this will need more memory and computation AND the developer time to implement the same sorted set solution. |
It is really unfortunate that this common requirement has to be re-implemented by each developer each time. I think Redis would be better off with this feature. |
+1 |
7 similar comments
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
1 similar comment
+1 |
I don't think expiration works as intended (or at least as I would expect it). The response keys are correctly expired, but nothing is every removed from the idempotency keys set. This has two consequences: - once a response key expires, requests using the same idempotency key will return 409 forever. - the idempotency keys set grows without bound Instead, I would expect expiration to also apply to the idempotency keys set. Once the expiration window has passed, a new request with the same idempotency key should issue a new request. This commit only introduces the tests without implementing a solution. For MemoryBackend, I'd propose storing the expiry alongside the idempotency key. For RedisBackend, I'd propose something along the lines of redis/redis#135 (comment) using a sorted set to purge expired idempotency keys. But I wanted to run the change by you before taking action on either approach.
I don't think expiration works as intended (or at least as I would expect it). The response keys are correctly expired, but nothing is ever removed from the idempotency keys set. This has two consequences: - once a response key expires, requests using the same idempotency key will return 409 forever. - the idempotency keys set grows without bound Instead, I would expect expiration to also apply to the idempotency keys set. Once the expiration window has passed, a new request with the same idempotency key should issue a new request. This commit only introduces the tests without implementing a solution. For MemoryBackend, I'd propose storing the expiry alongside the idempotency key. For RedisBackend, I'd propose something along the lines of redis/redis#135 (comment) using a sorted set to purge expired idempotency keys. But I wanted to run the change by you before taking action on either approach.
I don't think expiration works as intended (or at least as I would expect it to). The response keys are correctly expired, but nothing is ever removed from the idempotency keys set. This has two consequences: - once a response key expires, requests using the same idempotency key will return 409 forever - the idempotency keys set grows without bound Instead, I would expect expiration to also apply to the idempotency keys set. Once the expiration window has passed, a new request with the same idempotency key should issue a new request. This commit only introduces the tests without implementing a solution. For MemoryBackend, I'd propose storing the expiry alongside the idempotency key. For RedisBackend, I'd propose something along the lines of redis/redis#135 (comment) using a sorted set to purge expired idempotency keys. But I wanted to run the change by you before taking action on either approach.
I don't think expiration works as intended (or at least as I would expect it to). The response keys are correctly expired, but nothing is ever removed from the idempotency keys set. This has two consequences: - once a response key expires, requests using the same idempotency key will return 409 forever - the idempotency keys set grows without bound Instead, I would expect expiration to also apply to the idempotency keys set. Once the expiration window has passed, a new request with the same idempotency key should issue a new request. This commit only introduces the tests without implementing a solution. For MemoryBackend, I'd propose storing the expiry alongside the idempotency key. For RedisBackend, I'd propose something along the lines of redis/redis#135 (comment) using a sorted set to purge expired idempotency keys. But I wanted to run the change by you before taking action on either approach.
Previously, idempotency keys were never being expired. This had two consequences: - once a response key expires, requests using the same idempotency key will return 409 forever - the idempotency keys set grows without bound There's a new `expire_idempotency_keys` abstract method that implementations of Backend are responsible for implementing. The middleware calls it on every request. For the Memory backend, we now store idempotency keys in a dict intead of a set. The value of each key is its expiration time. We then iterate over all of the items in the dict and remove the ones with an expiration earlier than the current time. This operation isn't very efficient because it iterates over the entire dict, but given that the Memory backend is only meant to be used in local testing, I don't think it's worth optimizing further. For the RedisBackend, we now store idempotency keys in a sortedset instead of a set. The "score" of each key is its expiration time. This matches the Redis project's official recommendation on how to expire items in a set: redis/redis#135 (comment) We then delete any items from the sorted set with a "score" lower than the current time. The Redis docs describe this a potentially slow operation: https://redis.io/commands/zremrangebyscore/ If we don't want to call this on every request, we could instead add some sort of random fuzzing to only occassionally call it. Or we could abandon using a sorted set entirely and just store the idempotency key as its own key in Redis using the standard Redis expiration functionality. This would mean each request would store potentially three keys (idempotency key, response key, status code key) instead of the current two (response key, status code key). Users who upgrade to a version of the library that includes this change will see a failure in Redis when it tries to use a sortedset operation on an existing set. At a minimum, this should be called out in a changelog. Additionally, we could change the default idempotency key name from `idempotency-key-keys` to something else to avoid a collision.
Previously, idempotency keys were never being expired. This had two consequences: - once a response key expires, requests using the same idempotency key will return 409 forever - the idempotency keys set grows without bound There's a new `expire_idempotency_keys` abstract method that implementations of Backend are responsible for implementing. The middleware calls it on every request. For the Memory backend, we now store idempotency keys in a dict intead of a set. The value of each key is its expiration time. We then iterate over all of the items in the dict and remove the ones with an expiration earlier than the current time. This operation isn't very efficient because it iterates over the entire dict, but given that the Memory backend is only meant to be used in local testing, I don't think it's worth optimizing further. For the RedisBackend, we now store idempotency keys in a sortedset instead of a set. The "score" of each key is its expiration time. This matches the Redis project's official recommendation on how to expire items in a set: redis/redis#135 (comment) We then delete any items from the sorted set with a "score" lower than the current time. The Redis docs describe this a potentially slow operation: https://redis.io/commands/zremrangebyscore/ If we don't want to call this on every request, we could instead add some sort of random fuzzing to only occassionally call it. Or we could abandon using a sorted set entirely and just store the idempotency key as its own key in Redis using the standard Redis expiration functionality. This would mean each request would store potentially three keys (idempotency key, response key, status code key) instead of the current two (response key, status code key). Users who upgrade to a version of the library that includes this change will see a failure in Redis when it tries to use a sortedset operation on an existing set. At a minimum, this should be called out in a changelog. Additionally, we could change the default idempotency key name from `idempotency-key-keys` to something else to avoid a collision.
Previously, idempotency keys were never being expired. This had two consequences: - once a response key expires, requests using the same idempotency key will return 409 forever - the idempotency keys set grows without bound There's a new `expire_idempotency_keys` abstract method that implementations of Backend are responsible for implementing. The middleware calls it on every request. For the Memory backend, we now store idempotency keys in a dict intead of a set. The value of each key is its expiration time. We then iterate over all of the items in the dict and remove the ones with an expiration earlier than the current time. This operation isn't very efficient because it iterates over the entire dict, but given that the Memory backend is only meant to be used in local testing, I don't think it's worth optimizing further. For the RedisBackend, we now store idempotency keys in a sortedset instead of a set. The "score" of each key is its expiration time. This matches the Redis project's official recommendation on how to expire items in a set: redis/redis#135 (comment) We then delete any items from the sorted set with a "score" lower than the current time. The Redis docs describe this a potentially slow operation: https://redis.io/commands/zremrangebyscore/ If we don't want to call this on every request, we could instead add some sort of random fuzzing to only occassionally call it. Or we could abandon using a sorted set entirely and just store the idempotency key as its own key in Redis using the standard Redis expiration functionality. This would mean each request would store potentially three keys (idempotency key, response key, status code key) instead of the current two (response key, status code key). Users who upgrade to a version of the library that includes this change will see a failure in Redis when it tries to use a sortedset operation on an existing set. At a minimum, this should be called out in a changelog. Additionally, we could change the default idempotency key name from `idempotency-key-keys` to something else to avoid a collision.
Previously, idempotency keys were never being expired. This had two consequences: - once a response key expires, requests using the same idempotency key will return 409 forever - the idempotency keys set grows without bound There's a new `expire_idempotency_keys` abstract method that implementations of Backend are responsible for implementing. The middleware calls it on every request. For the Memory backend, we now store idempotency keys in a dict intead of a set. The value of each key is its expiration time. We then iterate over all of the items in the dict and remove the ones with an expiration earlier than the current time. This operation isn't very efficient because it iterates over the entire dict, but given that the Memory backend is only meant to be used in local testing, I don't think it's worth optimizing further. For the RedisBackend, we now store idempotency keys in a sortedset instead of a set. The "score" of each key is its expiration time. This matches the Redis project's official recommendation on how to expire items in a set: redis/redis#135 (comment) We then delete any items from the sorted set with a "score" lower than the current time. The Redis docs describe this a potentially slow operation: https://redis.io/commands/zremrangebyscore/ If we don't want to call this on every request, we could instead add some sort of random fuzzing to only occassionally call it. Or we could abandon using a sorted set entirely and just store the idempotency key as its own key in Redis using the standard Redis expiration functionality. This would mean each request would store potentially three keys (idempotency key, response key, status code key) instead of the current two (response key, status code key). Users who upgrade to a version of the library that includes this change will see a failure in Redis when it tries to use a sortedset operation on an existing set. At a minimum, this should be called out in a changelog. Additionally, we could change the default idempotency key name from `idempotency-key-keys` to something else to avoid a collision.
+1 |
1 similar comment
+1 |
+1 |
3 similar comments
+1 |
+1 |
+1 |
I created a module to achieve this https://github.com/Rush/redis_expiremember_module
It's not as good as a native implementation would be, but we use it with success |
I'd like the ability to expire members of a set instead of the whole set.
The patterns described here http://groups.google.com/group/redis-db/browse_thread/thread/ad75cc08b364352b are not really feasable to me.
The text was updated successfully, but these errors were encountered: