feat: Remove Last Cache Size Limitation#26333
Conversation
pauldix
left a comment
There was a problem hiding this comment.
Just a couple of small things
hiltontj
left a comment
There was a problem hiding this comment.
I left one comment in addition to Paul's - what I suggested isn't a blocking request though, but using NonZeroUsize (or NonZeroU64) might be useful for catching some corner cases but generally, the code as is should enforce the greater than 0 restrictions fine.
| /// Must be greater than 0 | ||
| #[derive(Debug, Serialize, Eq, PartialEq, Clone, Copy)] | ||
| pub struct LastCacheSize(pub(crate) usize); |
There was a problem hiding this comment.
This is a textbook use case for NonZeroUsize, i.e.,
/// Must be greater than 0
#[derive(Debug, Serialize, Eq, PartialEq, Clone, Copy)]
pub struct LastCacheSize(pub(crate) NonZeroUsize);There was a problem hiding this comment.
This looks like it'll be more involved given implementations across the DB will have to be updated; I can do that update, would want to do it in a separate chore though if that's alright.
If you've reconsidered and do think it should be blocking, let me know and I'll make the update.
There was a problem hiding this comment.
No worries. We can refactor this later
* feat: remove limit on LVC size * fix: bad test case and incorrect info * fix: more clarity and default value * fix: light CLI polishes * fix: bad snapshot
Removes limitation on cache size of 10 for Last Value Cache.
Additionally, closes #25522