Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

Timestamp fixes #915

Closed
wants to merge 9 commits into from
Closed

Timestamp fixes #915

wants to merge 9 commits into from

Conversation

Dieterbe
Copy link
Contributor

No description provided.

@Dieterbe Dieterbe requested review from replay and woodsaj May 17, 2018 18:46
@woodsaj
Copy link
Member

woodsaj commented May 18, 2018

this looks got to me. But the unit tests need to be fixed before this can be merged.

Dieterbe added 6 commits May 18, 2018 10:49
it happened to work because the only caller used an end that divides by
the step. But now it works in all cases
AggMetric, Store, and Cache all support retrieving a range of data.
ranges are defined as from to to; where from is inclusive and to is
exclusive. thus valid ranges must have from < to; with the special
case of from = to - 1, which selects exactly 1 1-second slot.

in particular:

* use uniform error messages across all 3
* AggMetric.Get(): when hitting unexpected nil chunk, return error.
  if we have corruption in our in-memory chunklist, i rather return
  an error to client rather than trying to query the store to prevent
  cascading trouble. Seems pretty rare considering chunkcache does
  most of the work, but the other reason is now that we return errors
  instead of panicing, this seems cleaner
* in cache.Search treat from > to as an error
if interval is, say 120s
then you can expect points with these timestamps:
120
240
360
...

previously, if you issued a render request such as to=300,until=330
on a raw archive (which still needs quantizing so we do some tricks)
those tricks would result in bad request errors.
Now we handle them as they should: return [].

fix #912
@Dieterbe Dieterbe mentioned this pull request May 18, 2018
@Dieterbe Dieterbe closed this May 18, 2018
@Dieterbe Dieterbe deleted the timestamp-fixes branch May 20, 2018 19:02
@Dieterbe Dieterbe mentioned this pull request May 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants