add partitioning of MetricData/MetricDefinitions #26

woodsaj · 2019-04-23T19:36:54Z

No description provided.

woodsaj · 2019-04-23T19:43:58Z

awoods@awoods-ThinkPad:~/go/src/github.com/raintank/schema$ go test -v -run none -bench BenchmarkPartitionBy -benchmem
goos: linux
goarch: amd64
pkg: github.com/raintank/schema
BenchmarkPartitionByOrg-8                       20000000                78.1 ns/op            16 B/op          3 allocs/op
BenchmarkPartitionBySeries-8                    20000000                60.8 ns/op            20 B/op          2 allocs/op
BenchmarkPartitionBySeriesWithTags-8             3000000               500 ns/op              32 B/op          1 allocs/op
BenchmarkPartitionBySeriesWithTagsFnv-8          1000000              1197 ns/op             488 B/op         23 allocs/op
PASS
ok      github.com/raintank/schema      6.511s

robert-milan · 2019-04-24T04:59:45Z

partition.go

+	case PartitionBySeriesWithTags:
+		h := xxhash.New()
+		h.WriteString(m.Name)
+		sort.Strings(m.Tags)


Should we perform a check to see if any tags exist? Even if tags don't exist I suppose we would still want to continue with the jump.Hash call on m.Name. I know in the end it doesn't make much of a difference computationally.

this procedure where the tags get sorted and then concatenated into a string (excluding name) is repeated multiple times, that could also just be in a function like getSortedTagString([]string) string

Should we perform a check to see if any tags exist?

we probably should not spend any effort on this or on seeing whether we should, as this is unlikely to be an issue

Even if tags don't exist I suppose we would still want to continue with the jump.Hash call on m.Name

yes, in that case it's equivalent to PartitionBySeries, which is what we want

this procedure where the tags get sorted and then concatenated into a string (excluding name) is repeated multiple times, that could also just be in a function like getSortedTagString([]string) string

+1

could also just be in a function like getSortedTagString([]string) string

We are deliberately optimising here to reduce allocations. We dont want to allocate memory for a new string only to then write that string to something else.

But we could use a func like writeSortedTagString(w io.StringWriter, []string) (n int, err error)

actually, that wont work, as we are using WriteString() when it is available and Write() otherwise.
so we would need to just use writeSortedTagString(w io.Writer, []string) (n int, err error) in which we can call io.WriteString(w, s) which will use the optimised WriteString() call if available and fall back to Write()

I implemented that and pushed into this branch

partition.go

Dieterbe

looks pretty good. but needs some more tweaks.
please update your commit message to mention why you're removing the Key methods (we ditch them and restrict ourselves to never setting keys anymore, in exchange for a simpler partitioning system)

robert-milan · 2019-06-12T12:04:49Z

metric.go

@@ -290,3 +247,30 @@ func ValidateTags(tags []string) bool {

 	return true
 }
+
+func writeSortedTagString(w io.Writer, name string, tags []string) error {


Will we be performing tag validation here? Or leave that up to tsdb-gw ?

There is a separate function to validate the tags, this is only supposed to write the name with tags into a writer

robert-milan · 2019-06-12T12:12:39Z

metric.go

-	KeyBySeries([]byte) []byte
+	// using the provided partitionByMethod, and number of partitions being used
+	// generate and return the partition id that should be used with this metric.
+	PartitionID(method PartitionByMethod, partitions int32) (int32, error)


Are there any other projects aside from Metrictank and tsdb-gw that use this interface? I suppose we will need to update them all at the same time. I know there are some PRs open to deal with it, but I don't think the MT PR is ready yet #grafana/metrictank/pull/1282

I'm only aware of TSDB and MT (in multiple locations). Once this is merged, MT and TSDB can update their vendored schema and start using the new partitioning methods, but there's nothing stopping us from merging this PR already before MT & TSDB integrated it.

there's nothing stopping us from merging this PR already before MT & TSDB integrated it.

in fact that's why the mentioned MT pr is stale, because it's waiting for this one to be done so it can pull this code in cleanly.

robert-milan · 2019-06-12T12:39:55Z

metric.go

@@ -136,27 +113,26 @@ func (m *MetricDefinition) NameWithTags() string {
 		return m.nameWithTags
 	}

-	sort.Strings(m.Tags)
+	nameWithTagsBuffer := &bytes.Buffer{}
+	writeSortedTagString(nameWithTagsBuffer, m.Name, m.Tags)


Are we going to check for the returned error here or not?

so, this is a bit funny. the only way how writeSortedTagString() would return an error would be if the first argument is an io.Writer that returned an error either from Write() or from WriteString(). Now, according to their signature, they can return an error. But in our code we only use 3 types of io.Writer, which are:

bytes.Buffer

xxhash.Digest

fnv.Hash32

None of these 3 can return an error when you look at their code, they are all hard-coded to only return nil as error. So I'm not sure if we really have to check for an error here. I wouldn't like to modify NameWithTags() so it returns an error, because then every location that uses it would also have to check for that error again etc...

writeSortedTagString() can't make assumptions about whether or not the passed in io.Writer actually will return an error or not, so it must check explicitly. (the current implementation does this right), and also return an error via its signature (regardless or whether it truly will at runtime or not)

I think the right solution here is that callers of writeSortedTagString() can choose to ignore the error if they know it'll never happen, which enables NameWithTags to not have to return an error in its signature. However:

we should be explicit / a bit more clear in the code. this seems to compile fine:

- writeSortedTagString(nameWithTagsBuffer, m.Name, m.Tags) + _ = writeSortedTagString(nameWithTagsBuffer, m.Name, m.Tags)

we can obviously only do this at callsites where we know the io.Writer won't actually return an error

as mauro points out, we only pass those 3 types of values into writeSortedTagsString and none of them returns errors.
this enables to change the signature of PartitionID to not return an error, at least if the caller promises to always provide a valid PartitionByMethod,.. i guess for now we can leave as is and revise later

This makes sense to me. We get the best of both versions, on one hand NameWithTags() does not return an error, on the other hand we still make sure that errors don't get swallowed.

metric.go

Dieterbe · 2019-07-09T13:08:36Z

I think this is ready for merging now.

replay · 2019-07-09T22:38:47Z

partition.go

+	return partition, nil
+}
+
+func (m *MetricDefinition) Partition(method PartitionByMethod, partitions int32) (int32, error) {


the MetricDefinition struct now has a property .Partition and a method .Partition()

replay

LGTM apart from that it doesn't work because of the name conflict
if that's fixed then it should be good to go

Co-Authored-By: Robert Milan <42070645+robert-milan@users.noreply.github.com>

Dieterbe · 2019-07-10T10:18:08Z

LGTM apart from that it doesn't work because of the name conflict

oops i got a bit quixotic again

fixed and rebased on master.

woodsaj requested review from Dieterbe and robert-milan April 23, 2019 19:36

woodsaj force-pushed the partitionBy branch from 51cefba to 2a7b77f Compare April 23, 2019 19:42

robert-milan reviewed Apr 24, 2019

View reviewed changes

Dieterbe reviewed May 13, 2019

View reviewed changes

partition.go Show resolved Hide resolved

Dieterbe reviewed May 13, 2019

View reviewed changes

partition.go Show resolved Hide resolved

Dieterbe suggested changes May 13, 2019

View reviewed changes

robert-milan reviewed Jun 12, 2019

View reviewed changes

replay force-pushed the partitionBy branch from fb5d565 to e5ba53e Compare June 12, 2019 14:25

Dieterbe force-pushed the partitionBy branch from a2f0117 to e7086c1 Compare July 9, 2019 12:30

replay reviewed Jul 9, 2019

View reviewed changes

replay approved these changes Jul 9, 2019

View reviewed changes

woodsaj and others added 7 commits July 10, 2019 12:13

add partitioning of MetricData/MetricDefinitions

f418f37

deduplicate NameWithTags string generation

b8bdc32

add some comments

e0f15bc

Adjust the name tag length check

9c28153

Co-Authored-By: Robert Milan <42070645+robert-milan@users.noreply.github.com>

better comments

7619e48

simpler

f4cd337

be explicit wrt error ignoring

36f89ce

Dieterbe force-pushed the partitionBy branch from d75daf3 to 36f89ce Compare July 10, 2019 10:15

Dieterbe merged commit d44495b into master Jul 10, 2019

Dieterbe deleted the partitionBy branch July 10, 2019 10:19

Dieterbe mentioned this pull request Jul 10, 2019

partitionBy bySeriesWithTags (aka "shard by tag") grafana/metrictank#1282

Closed

robert-milan mentioned this pull request Aug 13, 2019

Use new partition methods grafana/metrictank#1427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add partitioning of MetricData/MetricDefinitions #26

add partitioning of MetricData/MetricDefinitions #26

woodsaj commented Apr 23, 2019

woodsaj commented Apr 23, 2019

robert-milan Apr 24, 2019

replay Apr 24, 2019 •

edited

Loading

Dieterbe May 13, 2019

woodsaj May 15, 2019

woodsaj May 15, 2019

replay Jun 12, 2019

Dieterbe left a comment

robert-milan Jun 12, 2019

replay Jun 12, 2019

robert-milan Jun 12, 2019

replay Jun 12, 2019

Dieterbe Jul 9, 2019

robert-milan Jun 12, 2019

replay Jun 12, 2019 •

edited

Loading

Dieterbe Jul 9, 2019

Dieterbe Jul 9, 2019

replay Jul 9, 2019 •

edited

Loading

Dieterbe commented Jul 9, 2019

replay Jul 9, 2019

replay left a comment

Dieterbe commented Jul 10, 2019

add partitioning of MetricData/MetricDefinitions #26

add partitioning of MetricData/MetricDefinitions #26

Conversation

woodsaj commented Apr 23, 2019

woodsaj commented Apr 23, 2019

Choose a reason for hiding this comment

replay Apr 24, 2019 • 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

Dieterbe 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

replay Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

Dieterbe commented Jul 9, 2019

Choose a reason for hiding this comment

replay left a comment

Choose a reason for hiding this comment

Dieterbe commented Jul 10, 2019

replay Apr 24, 2019 •

edited

Loading

replay Jun 12, 2019 •

edited

Loading

replay Jul 9, 2019 •

edited

Loading