Use weekly tables in the chunk store #181

tomwilkie · 2017-01-04T11:47:10Z

Part of #158

Introduces a new job, the table manager, which periodically calculates the required tables (and provisioned throughputs) and then creates / updates the existing tables as appropriate.

Also update the chunk store to calculate the tables a given bucket should be written to.

TODO:

update bigBuckets to also pick a table for the bucket
new binary/container to create / update tables
unit test
add some monitoring for the dynamodb operations.
figure out when we can correctly remove write capacity from a table (see Flush chunks older than 12hrs #182)

- Make ASWSStore.bigBuckets emit tuples of (table name, bucket name), and plumb this through the write path. - New cortex_table_manager binary/image, which creates / updates the DynamoDB tables. - Make the table manager responsible for exporting capacities - Add unit tests and instrument the table manager - Don't remove provisioned write capacity until after max chunk age.

- fast start the table manage ticker - minimum write throughput is 1, not 0 - simplify the calculateExpectedTables functions, using int64 seconds (instead of time.Time) everywhere

jml

Thanks! This looks really solid. I've made enough comments that it's probably worth a second quick round of review.

jml · 2017-01-06T12:11:05Z

chunk/chunk_store.go

+	UsePeriodicTables    bool
+	TablePrefix          string
+	TablePeriod          time.Duration
+	PeriodicTableStartAt time.Time


Please add a comment for this block of configuration. IIUC, the rest of these aren't used if UsePeriodicTables is false—is that right?

Correct. 👍

jml · 2017-01-06T12:12:12Z

chunk/chunk_store.go

@@ -475,15 +407,15 @@ func (c *AWSStore) updateIndex(ctx context.Context, userID string, chunks []Chun
 		return err
 	}

-	return c.batchWriteDynamo(ctx, c.tableName, writeReqs)
+	return c.batchWriteDynamo(ctx, writeReqs)
 }

 // calculateDynamoWrites creates a set of batched WriteRequests to dynamo for all
 // the chunks it is given.
 //
 // Creates one WriteRequest per bucket per metric per chunk.


not necessarily - most of the time, buckets will live in the same table.

ah I see, it's "grouped by table".

jml · 2017-01-06T12:13:55Z

chunk/chunk_store.go

 }

 // calculateDynamoWrites creates a set of batched WriteRequests to dynamo for all
 // the chunks it is given.
 //
 // Creates one WriteRequest per bucket per metric per chunk.
-func (c *AWSStore) calculateDynamoWrites(userID string, chunks []Chunk) ([]*dynamodb.WriteRequest, error) {
-	writeReqs := []*dynamodb.WriteRequest{}
+func (c *AWSStore) calculateDynamoWrites(userID string, chunks []Chunk) (map[string][]*dynamodb.WriteRequest, error) {


I reckon adding a type alias for type tableName = string and type bucketName string and then using those in signatures like these would make the interactions between these methods easier to follow. Not strongly enough to insist on it though.

I thought about just plumbing the whole bucketSpec through... WDYT?

I like this better, thanks.

jml · 2017-01-06T12:17:28Z

chunk/chunk_store_test.go

 		}
 		buckets := cs.bigBuckets(s.from, s.through)
-		if !reflect.DeepEqual(buckets, s.buckets) {
+		if !reflect.DeepEqual(buckets, expected) {


Should probably add similar tests for when we're using the other bucketing strategy.

jml · 2017-01-06T12:18:20Z

chunk/chunk_store_test.go

+	}
+	if err := tableManager.syncTables(context.Background()); err != nil {
+		t.Fatal(err)
+	}


Maybe extract this repeated block to a helper function that makes a new manager and syncs tables, doing t.Fatal if it doesn't work.

jml · 2017-01-06T13:43:05Z

chunk/dynamo_table_manager.go

+
+	for i := firstTable; i <= lastTable; i++ {
+		table := tableDescription{
+			// Name construction needs to be consistent with chunk_store.bigBuckets


Thanks for adding this comment.

It's comments like these though that make me think we really should have a bucketing strategy interface with two distinct implementations. That way, all of the code that's required to be consistent between the manager and the chuck store can live in the same place—increasing the odds that it stays consistent. It also means less cyclomatic complexity in the implementations.

What would that interface look like? Note here we need something different to in chunk_store: we want all tables from the first to now. In chunk store we want the table for a given time.

Not 100% sure, and getting sure would probably require doing the refactoring myself.

jml · 2017-01-06T13:45:55Z

chunk/dynamo_table_manager.go

+	}); err != nil {
+		return nil, nil, err
+	}
+	sort.Strings(existingTables)


Please extract this to a method that lists existing tables.

jml · 2017-01-06T13:48:06Z

chunk/dynamo_table_manager.go

+	}
+	for ; i < len(descriptions); i++ {
+		toCreate = append(toCreate, descriptions[i])
+	}


Please extract this to a function / method that takes a list of existing tables and a list of descriptions, and returns the toCreate & toCheckThroughput lists.

Now we've broken out the listTables function, this is basically all thats left. Still want me to do it?

I do (it's easier to test & re-use that way!) but I can understand why you don't want to.

jml · 2017-01-06T13:48:48Z

chunk/dynamo_table_manager.go

+			i++
+			j++
+		}
+	}


Took me a while to figure out that this is essentially calculating the set difference. Not sure much can be done about that.

jml · 2017-01-06T13:49:27Z

chunk/dynamo_table_manager.go

+			// existingTables[j].name isn't in descriptions, can ignore
+			j++
+		} else {
+			// Table existis, need to check it has correct throughput


Nit: exists

jml

Thanks! Still some comments but at your discretion.

jml · 2017-01-06T16:35:04Z

chunk/dynamo_table_manager.go

+	ProvisionedReadThroughput  int64
+
+	// Not exported as only used by tests to inject mocks
+	dynamodb dynamodbClient


The MakeDynamoDbClient function can still be defined here. That'd keep main.go fairly small.

juliusv · 2017-01-09T12:14:45Z

Given that @jml has done a deeper review, I've just given this a 30-minute read to get an impression of how it all works. It looks overall good to me. The complexity that both the code and the system are reaching now scares me, but I don't have a simpler alternative to offer either. So a rough 👍 - and crazy that you can produce so much code that fast :)

juliusv · 2017-01-09T12:24:13Z

cmd/cortex_table_manager/main.go

+	dynamodbURL := flag.String("dynamodb.url", "localhost:8000", "DynamoDB endpoint URL.")
+	flag.StringVar(&cfg.TablePrefix, "dynamodb.periodic-table.prefix", "cortex_", "DynamoDB table prefix for the periodic tables.")
+	flag.DurationVar(&cfg.TablePeriod, "dynamodb.periodic-table.period", 7*24*time.Hour, "DynamoDB periodic tables period.")
+	flag.DurationVar(&cfg.CreationGracePeriod, "dynamodb.periodic-table.grace-period", 10*time.Minute, "DynamoDB periodic tables grace period (duration which table will be created/delete before/after its needed).")


its -> it's

delete -> deleted

tomwilkie self-assigned this Jan 4, 2017

tomwilkie force-pushed the weekly-tables branch 2 times, most recently from 1c4589c to 307dae6 Compare January 4, 2017 12:09

tomwilkie mentioned this pull request Jan 4, 2017

Daily buckets #180

Merged

tomwilkie force-pushed the weekly-tables branch 2 times, most recently from dbad6ec to c03660c Compare January 5, 2017 11:12

tomwilkie changed the title ~~WIP Weekly tables~~ [WIP] Use weekly tables in the chunk store Jan 5, 2017

tomwilkie changed the title ~~[WIP] Use weekly tables in the chunk store~~ Use weekly tables in the chunk store Jan 5, 2017

tomwilkie requested a review from jml January 5, 2017 12:12

tomwilkie mentioned this pull request Jan 5, 2017

Flush chunks older than 12hrs #182

Merged

tomwilkie added 2 commits January 5, 2017 17:12

Move dynamo client library indirection to separate file.

c28aa7f

tomwilkie force-pushed the weekly-tables branch 2 times, most recently from 8953bb9 to f4a338f Compare January 5, 2017 17:54

Tweaks from manual testing:

417edc4

- fast start the table manage ticker - minimum write throughput is 1, not 0 - simplify the calculateExpectedTables functions, using int64 seconds (instead of time.Time) everywhere

tomwilkie force-pushed the weekly-tables branch from f4a338f to 417edc4 Compare January 5, 2017 18:01

jml reviewed Jan 6, 2017

View reviewed changes

tomwilkie added 2 commits January 6, 2017 15:53

Review feedback

0ba746f

Review feedback (II)

030bdb8

jml approved these changes Jan 6, 2017

View reviewed changes

Review feedback III - factor out the dynamo/s3 client creation.

53b59c4

juliusv reviewed Jan 9, 2017

View reviewed changes

Review feedback IV - extend bigBuckets test to include tables.

da05254

tomwilkie merged commit 7efdad0 into master Jan 9, 2017

tomwilkie deleted the weekly-tables branch January 9, 2017 15:57

tomwilkie mentioned this pull request Jan 9, 2017

Prevent infinite loop in the table manager #187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use weekly tables in the chunk store #181

Use weekly tables in the chunk store #181

tomwilkie commented Jan 4, 2017 •

edited

Loading

jml left a comment

jml Jan 6, 2017

tomwilkie Jan 6, 2017

jml Jan 6, 2017

tomwilkie Jan 6, 2017 •

edited

Loading

jml Jan 6, 2017

jml Jan 6, 2017

tomwilkie Jan 6, 2017

tomwilkie Jan 6, 2017

jml Jan 6, 2017

jml Jan 6, 2017

jml Jan 6, 2017

tomwilkie Jan 6, 2017

jml Jan 6, 2017

tomwilkie Jan 6, 2017

jml Jan 6, 2017

jml Jan 6, 2017

tomwilkie Jan 6, 2017

jml Jan 6, 2017

tomwilkie Jan 6, 2017

jml Jan 6, 2017

jml Jan 6, 2017

tomwilkie Jan 6, 2017

jml Jan 6, 2017

tomwilkie Jan 6, 2017

jml left a comment

jml Jan 6, 2017

juliusv commented Jan 9, 2017

juliusv Jan 9, 2017

juliusv Jan 9, 2017

Use weekly tables in the chunk store #181

Use weekly tables in the chunk store #181

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

tomwilkie Jan 6, 2017 • edited Loading

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

jml left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliusv commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomwilkie commented Jan 4, 2017 •

edited

Loading

tomwilkie Jan 6, 2017 •

edited

Loading