Pull request for no-ap-in-storage #583
Conversation
The storage engine check twice that the granularity matches. Do it once only.
|
i don't really understand the second commit. why don't we have enough info to detect whether granularity or aggregation_method exists? |
This is how most storage driver works: for example the file driver tries to open the file for "mean/300s" and fails. It does not know that's because the metric does not exist or because last is not in the metric AP or because 300 is not in the metric AP. Therefore it checks for the directory of the metric to check if the metric exists, and then raises |
We often don't have enough information to distinguish the two and it does not matter in the end. Simplify the code base a bit by merging the two without losing information.
|
Pull-request updated, HEAD is now 2ec6f26 |
| raise AggregationDoesNotExist(metric, aggregation) | ||
| if granularity is None: | ||
| granularity = metric.archive_policy.definition[0].granularity | ||
| raise AggregationDoesNotExist(metric, aggregation, granularity) |
There was a problem hiding this comment.
ok. but it literally validates this right here and knows exactly if it's the granularity or the aggregate that is wrong... imo, if a storage driver does not find the actual corresponding file/object because it's yet to receive data, it should not return an error but return an empty timeseries becaues that's exactly what it is.
that said, i imagine this is because you are trying to address #548? is this the only way to handle it?
There was a problem hiding this comment.
No, I'm actually targeting #175. For that I need to remove archive policy usage from the storage driver and move the actual detection of invalid requests to the API layer.
Which mean even this code you quote is going away ASAP :)
There was a problem hiding this comment.
i see. so how do you envision 175 working? there is no validation and the user can submit any combination of granularity+aggregate? because if there's validation at some point prior to this, i'm thinking it's still better to just return empty ts rather than error.
There was a problem hiding this comment.
the user can submit any combination of granularity+aggregate?
Exactly.
i'm thinking it's still better to just return empty ts rather than error.
Actually, it's better to raise an error in storage and leave the layer above (e.g. REST API) decides what behaviour it wants to implement
That being said, this PR do not change the current behaviour of the API or even of storage – it merges 2 different exception into one. I feel like we're talking about something that is not there (yet).
There was a problem hiding this comment.
that's fair. i guess as long as we eventually move this validation stuff from storage then.
| raise AggregationDoesNotExist(metric, aggregation) | ||
| if granularity is None: | ||
| granularity = metric.archive_policy.definition[0].granularity | ||
| raise AggregationDoesNotExist(metric, aggregation, granularity) |
There was a problem hiding this comment.
that's fair. i guess as long as we eventually move this validation stuff from storage then.
|
Tests SUCCESS for HEAD 4f14e6a
|
ghost
merged commit 
storage: merge GranularityDoesNotExist and AggregationDoesNotExist
storage: remove double check of granularity