Allow switching to daily instead of hourly index buckets #178
Conversation
There can be slight overlap around the switchover time, but I deemed it safer (and not incorrect) than risking gaps due to some calculation error.
|
@tomwilkie This would be the idea - haven't tested it fully on Kubernetes yet, only the unit test, but will do when I get it all running again. |
| @@ -332,25 +338,45 @@ func (c *AWSStore) CreateTables() error { | |||
| return err | |||
| } | |||
|
|
|||
| func bigBuckets(from, through model.Time) []int64 { | |||
| func bigBuckets(from, through model.Time, dailyBucketsFrom model.Time) []string { | |||
There was a problem hiding this comment.
I'd make this a method on AWSStore and not pass dailyBucketsFrom as a param.
| if i > lastHourlyBucket { | ||
| break | ||
| } | ||
| result = append(result, strconv.Itoa(int(i))) |
There was a problem hiding this comment.
Not sure the conversion to int is safe here - they'll run out on 2038-01-19 IIRC. Should probably use https://golang.org/pkg/strconv/#FormatInt
There was a problem hiding this comment.
Ah ignore me, these are hours, so the range is huge.
|
Hmm, looks like leap second handling in unix time isn't what I thought it would be (surprise!): https://en.wikipedia.org/wiki/Unix_time#Leap_seconds. Unix time is noncontiguous. Given that we receive the samples with a unix-like ms timestamp from the client, not sure there is much we can do here. |
|
I'm pretty happy with this, so going to merge. I'm touching the same code as you, so this will save a bad merge. |
| @@ -206,6 +208,10 @@ type AWSStore struct { | |||
| tableName string | |||
| bucketName string | |||
|
|
|||
| // Around which time to start bucketing indexes by day instead of by hour. | |||
| // Only the day matters, not the time within the day. | |||
| dailyBucketsFrom model.Time | |||
There was a problem hiding this comment.
This variable is never initialised! Which means we'll default to weekly buckets for everything...
We should do two things: copy the config straight into the AWSStore struct, and change the meaning so the time-zero-value means no weekly buckets.
Will backout this PR.
|
Sorry about that - thus the remark that I hadn't tried it out yet ;) Hope it didn't cause too much havok (only auto-rolled-out to dev after merging, I assume). |
|
Yeah no worries, as much my bad for not catching / testing it myself before merging. |
Signed-off-by: Alex Le <[email protected]>
There can be slight overlap around the switchover time, but I deemed it
safer (and not incorrect) than risking gaps due to some calculation
error.
Part of #10