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

Delete Redis' secondary index on Limit deletion #378

Closed
alexsnaps opened this issue Oct 3, 2024 · 3 comments
Closed

Delete Redis' secondary index on Limit deletion #378

alexsnaps opened this issue Oct 3, 2024 · 3 comments
Labels
kind/bug Something isn't working

Comments

@alexsnaps
Copy link
Member

As of v1.6.0 we are not deleting the secondary index that stores all counter keys for a given Limit. That set key_for_counters_of_limit(limit) could be deleted once we succeeded deleting all the counters.

@alexsnaps alexsnaps added the kind/bug Something isn't working label Oct 3, 2024
@alexsnaps alexsnaps added this to the Limitador Server v2.0.0 milestone Oct 3, 2024
@alexsnaps alexsnaps moved this to Todo in Kuadrant Oct 3, 2024
@alexsnaps
Copy link
Member Author

alexsnaps commented Oct 3, 2024

I broke that like 2 years ago 🤦
That script would need to come back. Tho unsure about it...

// KEYS[1]: counter key
// KEYS[2]: key that contains the counters that belong to the limit
// ARGV[1]: counter max val
// ARGV[2]: counter TTL
// ARGV[3]: delta
pub const SCRIPT_UPDATE_COUNTER: &str = "
    local set_res = redis.call('set', KEYS[1], ARGV[1], 'EX', ARGV[2], 'NX')
    redis.call('incrby', KEYS[1], - ARGV[3])
    if set_res then
        redis.call('sadd', KEYS[2], KEYS[1])
    end";

// KEYS[1]: key with limits of namespace
// KEYS[2]: key with set of all namespaces
// ARGV[1]: limit
// ARGV[2]: namespace of the limit
pub const SCRIPT_DELETE_LIMIT: &str = "
    redis.call('srem', KEYS[1], ARGV[1])
    if redis.call('scard', KEYS[1]) == 0 then
        redis.call('srem', KEYS[2], ARGV[2])
    end
";

this redis.call('srem', KEYS[2], ARGV[2]) in SCRIPT_DELETE_LIMIT, shouldn't that be a del KEYS[2] ? Or was that maintaining the duplicate limits when they were stored in both locations (redis & config file)?
Anyways... I might have not broken it, but it's certainly not cleaned up today
cc/ @eguzki

@alexsnaps
Copy link
Member Author

So my bad, I didn't break this, the secondary was never maintained, that SCRIPT_DELETE_LIMIT deals with a complete different data structure 🤦 , sorry for muddying the water here.

@alexsnaps
Copy link
Member Author

Fixed in #383

@github-project-automation github-project-automation bot moved this from Todo to Done in Kuadrant Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
Status: Done
Development

No branches or pull requests

1 participant