Support providing an optional id to limits/counters #356
Conversation
chirino
commented
Jun 12, 2024
chirino
commented
- add a key_for_counters_v2 function that uses the id as the key if set, otherwise uses the previous key encoding strategy.
- updated the distributed store to use key_for_counters_v2. Since we can’t decode a partial counter from id based keys, we now also keep in memory the Counter in a counter field of the limits map.
| let key: CounterKey = counter.into(); | ||
| encoded_key = postcard::to_extend(&2u8, encoded_key).unwrap(); | ||
| encoded_key = postcard::to_extend(&key, encoded_key).unwrap() | ||
| } |
There was a problem hiding this comment.
A hard choice was made here... we prefix the encoding with 0x01 or 0x02 so that a future decoder knows which type of encoding was used. The problem is that this makes it incompatible with the existing key_for_counter encoding. I guess we could commit the encoded_key = postcard::to_extend(&2u8, encoded_key).unwrap(); line to remain compatible, but then the encoding is non-determinist. You could maybe even figure out how to craft an id so that it's encoding matches an existing key in the key_for_counter format.
limitador/src/limit.rs
Outdated
| @@ -307,6 +311,7 @@ where | |||
|
|
|||
| impl Limit { | |||
| pub fn new<N: Into<Namespace>, T: TryInto<Condition>>( | |||
| id: Option<String>, | |||
There was a problem hiding this comment.
Note to mostly myself here, as I'll probably pick this up, but...
Let's add a Limit::with_id(String, ..) or something, and keep ::new as is, implying the None. That would be less of change across all the files, and make I think for a cleaner API
There was a problem hiding this comment.
Good idea. ❤️ in flight wifi.
|
moving the id out of the constuctor drastically reduced the size of the change set. |
|
@alexsnaps redis keys will now encode using shorter binary encoding only when the id is set. |
* add a key_for_counters_v2 function that uses the id as the key if set, otherwise uses the previous key encoding strategy. * updated the distributed store to use key_for_counters_v2. Since we can’t decode a partial counter from id based keys, we now also keep in memory the Counter in a counter field of the limits map. * Use the shorter binary encoding for limits/counters when the id is set, continue to use the older text encoding for backward compatibility when the id is not set. Signed-off-by: Hiram Chirino <[email protected]>
|
after running the |
…sure they remain stable. Signed-off-by: Hiram Chirino <[email protected]>
|
Since I'm new to rust, I'm not sure about the "Should we push the version in our cargo.toml to 2.0.0-dev (and 1.0.0-dev for the crate)?" bit. I think this change does not break backward compact so I don't know if it needs a major rev. |
