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
force-pushed
the
254-label-in-hash-key
branch
2 times, most recently
from
a1b68bd to
97dc83d
Compare
tomwilkie
force-pushed
the
254-label-in-hash-key
branch
2 times, most recently
from
e304e0e to
ef4929c
Compare
jml
left a comment
jml
left a comment
There was a problem hiding this comment.
Just a quick look at chunk/schema.go for now. More to follow later.
chunk/schema.go
Outdated
There was a problem hiding this comment.
I'd rename this to GetWriteBuckets because it doesn't actually write buckets.
chunk/schema.go
Outdated
chunk/schema.go
Outdated
There was a problem hiding this comment.
Worth mentioning how / why this might fail?
chunk/schema.go
Outdated
chunk/schema.go
Outdated
There was a problem hiding this comment.
If we're going this far, why not type RangeKey = []byte (if that is the right thing)
chunk/schema.go
Outdated
chunk/schema.go
Outdated
chunk/schema.go
Outdated
There was a problem hiding this comment.
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.
chunk/schema.go
Outdated
There was a problem hiding this comment.
This deserves a doc comment saying what it's for.
chunk/schema.go
Outdated
There was a problem hiding this comment.
I don't have a constructive alternative (yet), but these panics point to the interface not being quite right.
tomwilkie
changed the title
chunk/schema.go
Outdated
There was a problem hiding this comment.
componsite -> composite
deletegates -> delegates
chunk/schema.go
Outdated
There was a problem hiding this comment.
Isn't it the lowest, not the highest, here?
There was a problem hiding this comment.
yes it is. I think code is correct; will update comment.
chunk/schema.go
Outdated
There was a problem hiding this comment.
If the found schema is the 0th, can this not become -1 and then the code below crashes (c.schemas[i])?
There was a problem hiding this comment.
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...
|
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.
tomwilkie
force-pushed
the
254-label-in-hash-key
branch
from
fae601f to
ee0abb5
Compare
|
👍 |
3 participants
Part of #254
TODO: