-
Notifications
You must be signed in to change notification settings - Fork 84
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
redis: use a Lua script to get measures #730
Conversation
Tests SUCCESS for HEAD 320a487
|
raise storage.AggregationDoesNotExist( | ||
metric, aggregation, sampling) | ||
if code == -2: | ||
raise storage.MetricDoesNotExist(metric) |
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.
i don't think we have tests for this as i deleted the following and it still passes.
+ if code == -1:
+ sampling = utils.to_timespan(result.split(self.FIELD_SEP_B)[2])
+ raise storage.AggregationDoesNotExist(
+ metric, aggregation, sampling)
+ if code == -2:
+ raise storage.MetricDoesNotExist(metric)
tbh, i'm not sure it's possible to get this far as we do validate the aggregation beforehand.
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.
I just kept the same structure, so IIUC, this is not a new problem (if it is one at all)?
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.
well it's a problem in that the errors raise are never validated so in theory, you could've broken it :P
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.
create a bug or something?
Tests SUCCESS for HEAD 16bf7bc
|
No description provided.