Daily buckets #180

tomwilkie · 2017-01-04T11:42:41Z

Second attempt at #178, part of #10

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.

jml

I think it's "Approve", but Github makes it too hard to review all the comments. Ping me when you've replied and I'll try to follow up promptly.

jml · 2017-01-04T13:54:11Z

chunk/chunk_store.go

@@ -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


Can we model this with a type that only specifies day?

We can make it an int, but this just pushes the problem into main.go. I think I prefer it like this.

Understood. I had to dig into the model code to make sense of that. It's just an alias to a number of milliseconds, rather than, say, a struct. I guess we could make our own type alias, Day, to make it clearer, but not worth it IMO.

jml · 2017-01-04T13:54:33Z

chunk/chunk_store.go

@@ -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.


Before or after this time?

After this time; will update comment.

jml · 2017-01-04T13:57:23Z

chunk/chunk_store.go

+		fromDay      = from.Unix() / secondsInDay
+		throughDay   = through.Unix() / secondsInDay
+
+		firstDailyBucket = c.dailyBucketsFrom.Unix() / secondsInDay


This might be a personal taste thing, but I'd just make dailyBucketsFrom a parameter. Would reduce total LoC because you wouldn't have to construct an AWSStore in your tests. Up to you though.

My comment to @juliusv was the exact opposite - mainly because of whats coming in #181.

jml · 2017-01-04T13:58:39Z

chunk/chunk_store.go

@@ -332,25 +339,45 @@ func (c *AWSStore) CreateTables() error {
 	return err
 }

-func bigBuckets(from, through model.Time) []int64 {
+func (c *AWSStore) bigBuckets(from, through model.Time) []string {


I reckon this function / method could use a comment saying what it does. Especially, it should give examples of the expected bucket strings. Or point to the tests.

jml · 2017-01-04T14:05:32Z

chunk/chunk_store_test.go

@@ -146,3 +146,44 @@ func diff(want, have interface{}) string {
 	})
 	return "\n" + text
 }
+
+func TestBigBuckets(t *testing.T) {


Really glad you wrote tests!

I found them a bit hard to understand their purpose at first, so I'm going to suggest some commentary inside.

jml · 2017-01-04T14:15:18Z

chunk/chunk_store_test.go

+			dailyBucketsFrom: model.TimeFromUnix(0).Add(1*24*time.Hour) - 1,
+			buckets:          []string{"0", "d0", "d1", "d2"},
+		},
+		{


// If `dailyBucketsFrom` is after the interval, everything will be bucketed by hour.

jml · 2017-01-04T14:15:43Z

chunk/chunk_store_test.go

+			through:          model.TimeFromUnix(0).Add(2 * 24 * time.Hour),
+			dailyBucketsFrom: model.TimeFromUnix(0).Add(99 * 24 * time.Hour),
+			buckets:          []string{"0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18", "19", "20", "21", "22", "23", "24", "25", "26", "27", "28", "29", "30", "31", "32", "33", "34", "35", "36", "37", "38", "39", "40", "41", "42", "43", "44", "45", "46", "47", "48"},
+		},


Maybe add another case that checks when dailyBucketsFrom is before the interval?

jml · 2017-01-04T14:16:58Z

chunk/chunk_store.go

 	var (
 		secondsInHour = int64(time.Hour / time.Second)
 		fromHour      = from.Unix() / secondsInHour
 		throughHour   = through.Unix() / secondsInHour
-		result        []int64
+
+		secondsInDay = int64(24 * time.Hour / time.Second)


I know Go is a bit fiddly about this, but wdyt about making secondsInDay and secondsInHour consts?

jml · 2017-01-04T14:17:48Z

chunk/chunk_store.go

+		if i > lastHourlyBucket {
+			break
+		}
+		result = append(result, strconv.Itoa(int(i)))


Should these be prefixed with h? Or is there a constraint which prevents that?

Backwards compatibility prevents us doing that.

jml · 2017-01-04T14:19:48Z

cmd/cortex/main.go

@@ -246,10 +249,15 @@ func setupChunkStore(cfg cfg) (chunk.Store, error) {
 			Expiration: cfg.memcachedExpiration,
 		}
 	}
+	dailyBucketsFrom, err := time.Parse("2006-01-02", cfg.dynamodbDailyBucketsFrom)


"2006-12-31" might communicate intent a bit better.

thats not how go date parsing works - https://golang.org/pkg/time/#Parse

The layout defines the format by showing how the reference time, defined to be:

Mon Jan 2 15:04:05 -0700 MST 2006

Oh wow, OK. That's ... interesting.

tomwilkie · 2017-01-04T15:13:19Z

@jml PTAL

jml

Thanks!

jml · 2017-01-04T15:21:33Z

chunk/chunk_store_test.go

+			buckets:          []string{"0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18", "19", "20", "21", "22", "23", "24", "d1", "d2"},
+		},
+
+		{


Can you please add a comment describing the behaviour this is testing?

we'll have to wait for @juliusv next week

Hmm, I guess I just wanted to show that 24 1-hour buckets disappear when you make the dailyBucketsFriom one day earlier than in the case above (but the one overlapping 1h bucket still remains). So maybe the following?

"// Moving dailyBucketsFrom to the previous day compared to the above makes 24 1-hour buckets disappear."

But yeah, the incremental value of this test case is perhaps debatable :)

juliusv and others added 3 commits January 4, 2017 11:39

Allow switching to daily instead of hourly index buckets

6829e1b

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.

Make bigBuckets a method of AWSStore

bb27555

Initialise dailyBucketsFrom

299f260

jml reviewed Jan 4, 2017

View reviewed changes

Review feedback

fa36d2b

jml approved these changes Jan 4, 2017

View reviewed changes

Add extra comment.

15eb5b7

tomwilkie merged commit 3592422 into master Jan 5, 2017

tomwilkie deleted the daily-buckets branch January 5, 2017 11:07

tomwilkie mentioned this pull request Jan 5, 2017

Switch to varbit chunk encoding to improve compression #130

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Daily buckets #180

Daily buckets #180

tomwilkie commented Jan 4, 2017 •

edited

Loading

jml left a comment

jml Jan 4, 2017

tomwilkie Jan 4, 2017

jml Jan 4, 2017

jml Jan 4, 2017

tomwilkie Jan 4, 2017

tomwilkie Jan 4, 2017

jml Jan 4, 2017

tomwilkie Jan 4, 2017

jml Jan 4, 2017

tomwilkie Jan 4, 2017

tomwilkie Jan 4, 2017

jml Jan 4, 2017

tomwilkie Jan 4, 2017

jml Jan 4, 2017

jml Jan 4, 2017

tomwilkie Jan 4, 2017

jml Jan 4, 2017

tomwilkie Jan 4, 2017

tomwilkie Jan 4, 2017

jml Jan 4, 2017

tomwilkie Jan 4, 2017

jml Jan 4, 2017

tomwilkie Jan 4, 2017

jml Jan 4, 2017

tomwilkie commented Jan 4, 2017

jml left a comment

jml Jan 4, 2017

tomwilkie Jan 4, 2017

juliusv Jan 4, 2017

Daily buckets #180

Daily buckets #180

Conversation

tomwilkie commented Jan 4, 2017 • edited Loading

jml left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomwilkie commented Jan 4, 2017

jml left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomwilkie commented Jan 4, 2017 •

edited

Loading