feat: add optional per-collection storage limits#2450
Conversation
chenba
force-pushed
the
feat/per-collection-storage-limit-stor-587
branch
from
939add0 to
ba6d97e
Compare
This commit: - adds a third field to the elements of STD_COLLS for per-collection storage limit - adds an optional `limits.collections` map in settings that overrides the default limits from STD_COLLS - enforces the per-collection limits separately from account-wide usage
chenba
force-pushed
the
feat/per-collection-storage-limit-stor-587
branch
from
ba6d97e to
e2c6d54
Compare
| fn set_quota(&mut self, enabled: bool, limit: usize, enforce: bool); | ||
|
|
||
| #[cfg(debug_assertions)] | ||
| fn set_collection_limits(&mut self, _limits: std::collections::HashMap<i32, usize>) {} |
There was a problem hiding this comment.
Had a thought that since we're destructuing all over in the format (for example):
if !STD_COLLS.iter().any(|(_, std_name, _)| std_name == name) {
might be nice to add tidy named struct (Ex. struct StdColl { id, name, default_limit }) that would read better and survive the next field addition without touching every call site. Can be left as-is, but occurred to me as a possible improvement.
| } | ||
|
|
||
| // Per-collection limits may only target standard collections in STD_COLLS. | ||
| for name in self.syncstorage.limits.collections.keys() { |
There was a problem hiding this comment.
we check the name here but never the value. a limit of 0 gets accepted and then every write to that collection trips the ceiling, since the check is >=. Maybe can we reject 0, and warn when a limit is larger than max_request_bytes so it can never actually be hit in one request?
| if !collection_limits.is_empty() | ||
| && let Some(obj) = limits_value.as_object_mut() | ||
| { | ||
| obj.insert( |
There was a problem hiding this comment.
this advertises the collection limits whenever enable_quota is on, but they only get enforced with enforce_quota. so during the observe phase a client sees a limit we won't actually reject on. Maybe a sentence in config.md so nobody reads the advertised number as a hard guarantee or that it is confusing?
| self.coll_limits.get(&collection_id).copied() | ||
| } | ||
|
|
||
| pub(super) fn emit_at_limit(&self, collection: &str, total_bytes: usize) -> DbResult<()> { |
There was a problem hiding this comment.
I believe this shifts spanner behavior on the batch path. before we only emitted storage.quota.at_limit when enforcing. now we always emit, then warn. lines spanner up with mysql and pg which is good, but a warn only deployment will start emitting this metric where it didn't before. something to keep in mind for our logging.
2 participants
This commit:
limits.collectionsmap in settings that overrides the default limits from STD_COLLSCloses STOR-587