-
Notifications
You must be signed in to change notification settings - Fork 23
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
First step: Limit storage #352
Conversation
fn get_counters(&self, limits: &HashSet<Arc<Limit>>) -> Result<HashSet<Counter>, StorageErr>; // todo revise typing here? | ||
fn delete_counters(&self, limits: &HashSet<Arc<Limit>>) -> Result<(), StorageErr>; // todo revise typing here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking of making these &[Arc<Limit>]
, and storing the limits in a ordered Vec<Limit>
, where same namespace Limit
s would be adjacent to each other... Still running tests on this tho. I think having a slice here is acceptable, might not convey the contract as clearly as a HashSet
wrt uniqueness, but would be acceptable as a tradeoff if the perf improvement is there... And for "other cases" one would need to create the Vec
on the callsite.
async fn get_counters( | ||
&self, | ||
limits: &HashSet<Arc<Limit>>, | ||
) -> Result<HashSet<Counter>, StorageErr>; | ||
async fn delete_counters(&self, limits: &HashSet<Arc<Limit>>) -> Result<(), StorageErr>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment above on the sync version of the trait.
@@ -226,6 +226,49 @@ pub struct CheckResult { | |||
pub limit_name: Option<String>, | |||
} | |||
|
|||
impl CheckResult { | |||
pub fn response_header(&mut self) -> HashMap<String, String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking this should return something other than String
as keys to our Map
here, but not a big deal (I think) for now as there is only one supported impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for refactoring this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Slowly refactoring this, so the idea is that:
Limit
definitions are now stored inArc
, so that they can be shared within the crate's boundaries (i.e. towards the(Async)CounterStorage
d040c0dCounter
as part of the lib's API, only as part of the SPI, i.e.(Async)CounterStorage
eefd5d4Counter.limit
be anArc
coming from theStorage.limit
's eefd5d4I'll then move on to fixing #328