Skip to content

Don't fetch chunks outside our timerange#149

Merged
tomwilkie merged 2 commits intomasterfrom
dont-read-chunks-out-of-range
Nov 23, 2016
Merged

Don't fetch chunks outside our timerange#149
tomwilkie merged 2 commits intomasterfrom
dont-read-chunks-out-of-range

Conversation

@tomwilkie
Copy link
Contributor

Should help with #132

return nil, err
}
if chunkThrough < from || through < chunkFrom {
fmt.Println(chunkThrough, from, through, chunkFrom)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing this Println is there for debugging. Please remove or turn into a proper log call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops!

queryChunks.Observe(float64(len(missing)))
filtered := make([]Chunk, 0, len(chunks))
for _, chunk := range chunks {
_, chunkFrom, chunkThrough, err := parseChunkID(chunk.ID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realise the from & through terminology was present before this PR, but I think start & end would be better. Up to you whether you want to change in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually like from & through - end tends to be a reserved word in many languages, and through implies inclusivity.

@tomwilkie tomwilkie mentioned this pull request Nov 23, 2016
@tomwilkie tomwilkie merged commit 3f72a81 into master Nov 23, 2016
@tomwilkie tomwilkie deleted the dont-read-chunks-out-of-range branch November 23, 2016 14:05
@tomwilkie tomwilkie mentioned this pull request Jan 13, 2017
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants