This repository was archived by the owner on Aug 23, 2023. It is now read-only.
Timestamp fixes etc #916
Closed
Timestamp fixes etc #916
+309
−161
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
force-pushed
the
timestamp-fixes-etc
branch
from
7212592 to
c007678
Compare
replay
reviewed
May 18, 2018
mdata/cache/ccache.go
Outdated
| @@ -209,8 +212,8 @@ func (c *CCache) Search(ctx context.Context, metric schema.AMKey, from, until ui | |||
| Until: until, | |||
| } | |||
|
|
|||
| if from == until { | |||
| return res | |||
| if from >= until { | |||
There was a problem hiding this comment.
actually this could happen before CCSearchResult is instantiated
replay
reviewed
May 18, 2018
store/cassandra/store_cassandra.go
Outdated
| errReadTooOld = errors.New("the read is too old") | ||
| errTableNotFound = errors.New("table for given TTL not found") | ||
| errCtxCanceled = errors.New("context canceled") | ||
| errChunkTooSmall = errors.New("unpossibly small chunk in cassandra") |
There was a problem hiding this comment.
shouldn't that be impossibly? is unpossibly a word?
replay
approved these changes
May 18, 2018
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I think something's up with circle.
this replaces #915