Make the schemas declarative, add a new schema for better load balancing. #262

tomwilkie · 2017-02-02T16:20:25Z

Part of #254

Define a Schema interface for calculating the hash and range keys to use.
Make implementations of this interface for all the different schema's we've had.
Make a "CompositeSchema" which delegates to various schema's depending on when they we activated.
Update chunk store to use this; in particular, invert the order of the merge and intersect in the query path.
Bunch of testing

TODO:

Add new scheme with the label name part of the hash key, hopefully improving the load balancing across DynamoDB partitions.
more testing for range keys
test v4 schema
adapt chunk store test to use each schema
figure out when its safe to read base64 only
- deployed to dev on Jan 17th https://github.com/weaveworks/service-conf/commit/d5535139413137b71740d1e925b09052690eaf18
- deployed to prod on Jan 18th https://github.com/weaveworks/service-conf/pull/510

jml

Just a quick look at chunk/schema.go for now. More to follow later.

jml · 2017-02-03T15:49:41Z

chunk/schema.go

+// to write or read chunks from the external index.
+type Schema interface {
+	// When doing a write, use this method to return the list of Buckets you should write to.
+	WriteBuckets(from, through model.Time, userID string, metricName model.LabelValue) []WriteBucket


I'd rename this to GetWriteBuckets because it doesn't actually write buckets.

jml · 2017-02-03T15:49:48Z

chunk/schema.go

+
+	// When doing a read, use these method to return the list of Buckets you should query
+	ReadBucketsForMetric(from, through model.Time, userID string, metricName model.LabelValue) []ReadBucket
+	ReadBucketsForMetricLabel(from, through model.Time, userID string, metricName model.LabelValue, labelName model.LabelName) []ReadBucket


Same, GetRead...

jml · 2017-02-03T15:50:28Z

chunk/schema.go

+	TableName() string
+	StartTime() model.Time
+
+	// For writing, pass metrics here and we'll tell you what range keys to write


Worth mentioning how / why this might fail?

jml · 2017-02-03T15:51:04Z

chunk/schema.go

+	HashKey() string
+	StartTime() model.Time
+
+	// For reading from this bucket, use this functions to construct a key that will prefix match the desired chunks


s/this/these/

jml · 2017-02-03T15:51:48Z

chunk/schema.go

+	StartTime() model.Time
+
+	// For reading from this bucket, use this functions to construct a key that will prefix match the desired chunks
+	AllLabelsPrefix() ([]byte, error)


If we're going this far, why not type RangeKey = []byte (if that is the right thing)

jml · 2017-02-03T15:53:21Z

chunk/schema.go

+	}
+
+	if !sort.IsSorted(byStart(schemas)) {
+		fmt.Printf("%+v", schemas)


jml · 2017-02-03T15:53:56Z

chunk/schema.go

+			return a
+		}
+		return b
+	}


jml · 2017-02-03T15:57:24Z

chunk/schema.go

+			nextSchemaStarts = c.schemas[i+1].start
+		}
+		end := min(through, nextSchemaStarts)
+		callback(start, end, c.schemas[i].Schema)


The callback thing seems a bit weird. I can see how it leads to briefer code below, but surely defining a struct and returning a slice / sending to a channel is more traditional.

jml · 2017-02-03T15:57:40Z

chunk/schema.go

+	f.Var(&cfg.V4SchemaFrom, "dynamodb.v4-schema-from", "The date (in the format YYYY-MM-DD) after which we enable v4 schema.")
+}
+
+type compositeSchema struct {


This deserves a doc comment saying what it's for.

jml · 2017-02-03T16:19:02Z

chunk/schema.go

+func (b *originalBucket) SpecificValuePrefix(value model.LabelValue) ([]byte, error) {
+	if b.labelName == "" {
+		panic("SpecificValuePrefix called on bucket not bound to specific label!")
+	}


I don't have a constructive alternative (yet), but these panics point to the interface not being quite right.

juliusv · 2017-02-06T11:26:25Z

chunk/schema.go

+	return cfg.TablePrefix + strconv.Itoa(int(bucketStart/int64(cfg.TablePeriod/time.Second)))
+}
+
+// componsiteSchema is a Schema which deletegates to various schemas depending


componsite -> composite
deletegates -> delegates

juliusv · 2017-02-06T11:31:13Z

chunk/schema.go

+	})
+	i--
+
+	// next, find the schema with the highest start _after_ through


Isn't it the lowest, not the highest, here?

yes it is. I think code is correct; will update comment.

juliusv · 2017-02-06T11:34:59Z

chunk/schema.go

+	i := sort.Search(len(c.schemas), func(i int) bool {
+		return c.schemas[i].start > from
+	})
+	i--


If the found schema is the 0th, can this not become -1 and then the code below crashes (c.schemas[i])?

True; in reality I didn't think this would happen (as we stick a schema in at 0), but I guess someone could send a sample from before 1970 and crash us...

juliusv · 2017-02-06T11:46:00Z

Looks pretty good to me besides minor comments!

- Define a Schema interface for calculating the hash and range keys to use. - Make implementations of this interface for all the different schema's we've had. - Make a "CompositeSchema" which delegates to various schema's depending on when they we activated. - Update chunk store to use this; in particular, invert the order of the merge and intersect in the query path. - Add v4 schema, which puts the label key in the hash key for better load distribution - Remove query related-metrics, they're not used. Bunch of testing: - Test the hash key generators, range key generators and composite scheme - Run the chunk store tests against each schema. - Resolve issue found from extending the tests - don't check results match label name, thats implied.

- DynamoDB won't accept empty keys; they should just be omitted. - Deal with case where two schemas start at the same time.

juliusv · 2017-02-06T14:43:51Z

👍

tomwilkie self-assigned this Feb 2, 2017

tomwilkie force-pushed the 254-label-in-hash-key branch 2 times, most recently from a1b68bd to 97dc83d Compare February 2, 2017 20:58

tomwilkie mentioned this pull request Feb 3, 2017

Ingester flush queue is backing up again #254

Closed

9 tasks

tomwilkie force-pushed the 254-label-in-hash-key branch 2 times, most recently from e304e0e to ef4929c Compare February 3, 2017 16:11

jml reviewed Feb 3, 2017

View reviewed changes

tomwilkie changed the title ~~[WIP] Make the schemas declarative.~~ Make the schemas declarative, add a new schema for better load balancing. Feb 5, 2017

juliusv reviewed Feb 6, 2017

View reviewed changes

tomwilkie added 2 commits February 6, 2017 14:42

Fix a couple of bugs from manual testing:

ee0abb5

- DynamoDB won't accept empty keys; they should just be omitted. - Deal with case where two schemas start at the same time.

tomwilkie force-pushed the 254-label-in-hash-key branch from fae601f to ee0abb5 Compare February 6, 2017 14:42

tomwilkie merged commit f66f505 into master Feb 6, 2017

tomwilkie deleted the 254-label-in-hash-key branch February 6, 2017 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the schemas declarative, add a new schema for better load balancing. #262

Make the schemas declarative, add a new schema for better load balancing. #262

tomwilkie commented Feb 2, 2017 •

edited

Loading

jml left a comment

jml Feb 3, 2017

jml Feb 3, 2017

jml Feb 3, 2017

jml Feb 3, 2017

jml Feb 3, 2017

jml Feb 3, 2017

jml Feb 3, 2017

jml Feb 3, 2017

jml Feb 3, 2017

jml Feb 3, 2017

juliusv Feb 6, 2017

juliusv Feb 6, 2017

tomwilkie Feb 6, 2017

juliusv Feb 6, 2017

tomwilkie Feb 6, 2017

juliusv commented Feb 6, 2017

juliusv commented Feb 6, 2017

Make the schemas declarative, add a new schema for better load balancing. #262

Make the schemas declarative, add a new schema for better load balancing. #262

Conversation

tomwilkie commented Feb 2, 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

juliusv commented Feb 6, 2017

juliusv commented Feb 6, 2017

tomwilkie commented Feb 2, 2017 •

edited

Loading