simplify store month calculations #1148
Conversation
Dieterbe
force-pushed
the
simplify-month-calculations
branch
from
74f01c9 to
aae24d0
Compare
Dieterbe
changed the title
| @@ -433,17 +433,14 @@ func (c *CassandraStore) SearchTable(ctx context.Context, key schema.AMKey, tabl | |||
|
|
|||
| pre := time.Now() | |||
|
|
|||
| start_month := start - (start % Month_sec) // starting row has to be at, or before, requested start | |||
| end_month := (end - 1) - ((end - 1) % Month_sec) // ending row has to include the last point we might need (end-1) | |||
|
|
|||
| // unfortunately in the database we only have the t0's of all chunks. | |||
| // this means we can easily make sure to include the correct last chunk (just query for a t0 < end, the last chunk will contain the last needed data) | |||
| // but it becomes hard to find which should be the first chunk to include. we can't just query for start <= t0 because than we will miss some data at | |||
| // the beginning. We can't assume we know the chunkSpan so we can't just calculate the t0 >= (start - <some-predefined-number>) because chunkSpans | |||
| // may change over time. | |||
| // we effectively need all chunks with a t0 > start, as well as the last chunk with a t0 <= start. | |||
There was a problem hiding this comment.
shouldn't this be as well as the last chunk with a t0 <= end instead of start
store/cassandra/cassandra.go
Outdated
| @@ -433,17 +433,14 @@ func (c *CassandraStore) SearchTable(ctx context.Context, key schema.AMKey, tabl | |||
|
|
|||
| pre := time.Now() | |||
|
|
|||
| start_month := start - (start % Month_sec) // starting row has to be at, or before, requested start | |||
| end_month := (end - 1) - ((end - 1) % Month_sec) // ending row has to include the last point we might need (end-1) | |||
|
|
|||
| // unfortunately in the database we only have the t0's of all chunks. | |||
| // this means we can easily make sure to include the correct last chunk (just query for a t0 < end, the last chunk will contain the last needed data) | |||
| // but it becomes hard to find which should be the first chunk to include. we can't just query for start <= t0 because than we will miss some data at | |||
| for month := start_month; month <= end_month; month += cassandra.Month_sec { | ||
| row_key := fmt.Sprintf("%s_%d", metric.AMKey.String(), month/cassandra.Month_sec) | ||
| for num := startMonth; num <= endMonth; num += 1 { | ||
| row_key := fmt.Sprintf("%s_%d", metric.AMKey.String(), num) |
There was a problem hiding this comment.
is .String() necessary? I thought it calls that automatically
There was a problem hiding this comment.
https://golang.org/pkg/fmt/#hdr-Printing basically says that if it implements Error and String that Error will be called first, so we should probably make it a habit to explicitly call String(). I haven't read the entire thing yet so if I find any other information I'll re-post it here.
There was a problem hiding this comment.
an Error() method only makes sense on types that represent errors or are tightly bound to an error (e.g. iterator api's sometimes have a method to retrieve an error, if any).
but for our key types, this clearly doesn't apply, so i'm following replay's suggestion.
There was a problem hiding this comment.
we voted on slack it's better to always be explicit for every type.
see #1188
see new rule introduced in #1188
small simplification of code. should be 100% equivalent to what it was before, by leveraging the fact that integer divisions are always rounded down.