Kuadrant / limitador

Rate limiter
Apache License 2.0
60 stars 21 forks source link

Delete Redis' secondary index on Limit deletion #378

Closed alexsnaps closed 1 month ago

alexsnaps commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Fixed in #383