Add partitioned index #1232

woodsaj · 2019-03-07T11:36:53Z

Add a PartitionedIndex wrapper around the memoryIdx.
Partitioning the index can be toggled via a config flag. By default it is off.

shanson7 · 2019-03-07T12:10:14Z

Looks interesting. Is the goal of this component to reduce lock contention? Can you share any benchmarks run?

robert-milan

LGTM

idx/memory/memory.go

idx/memory/partitioned_idx.go

replay · 2019-03-07T14:49:05Z

if we already partition by partition id, should we also partition by org id? in the case of multi tenant setups this would keep the defById map smaller and it seems like it should be easy to now just make the key something like (orgId * 128) + partition assuming the max number of partitions is 128.

idx/memory/partitioned_idx.go

replay · 2019-03-07T15:16:29Z

idx/memory/partitioned_idx.go

+	result := make([][]idx.Archive, len(p.Partition))
+	var i int
+	for _, m := range p.Partition {
+		pos, m := i, m


same as above^^

idx/memory/partitioned_idx.go

woodsaj · 2019-03-07T20:36:10Z

thanks for the review @replay
I think this should be good to merge. The change is pretty safe given that by default the unpartitionedIdx will be used.

relevant benchmark

goos: linux
goarch: amd64
pkg: github.com/grafana/metrictank/idx/memory
BenchmarkConcurrentInsertFind/partitioned-8                  10000            363256 ns/op          524709 B/op      11788 allocs/op
BenchmarkConcurrentInsertFind/unPartitioned-8               3000            333891 ns/op          484942 B/op      10115 allocs/op
PASS
ok      github.com/grafana/metrictank/idx/memory        12.061s

For most benchmarks the the PartitionedMemoryIdx performs worse than the unpartitioned version. This is all due to the results from searching all partitions needing to be merged, which results in lots of allocations.

The next step is to continue exposing "partitions" throughout the code base. The end goal is to have all partitions handled independently and allow peers to query specific partitions. This will allow nodes to respond with data for some partitions while others are blocked. The speculative execution can then try alternate peers for partitions that are slow.

- add config flag to enable partitionedIndex - update cassandra and bigtable indexes to use MemoryIndex interface - update all unit tests to run against both MemoryIdx and PartitionedMemoryIdx

this is just to make response responses consistent between MemoryIdx and PartitionedMemoryIdx

replay · 2019-03-08T10:30:06Z

@woodsaj sounds good. I'd just like to ask first what you think about my above suggestion, because i feel like this could help a lot for multi tenant setups with many tenants:

if we already partition by partition id, should we also partition by org id? in the case of multi tenant setups this would keep the defById map smaller and it seems like it should be easy to now just make the key something like (orgId * 128) + partition assuming the max number of partitions is 128.

Instead of keeping it a single map of index partitions we could also add a second level like Partition[orgId][partitionId]*UnpartitionedMemoryIdx instead of Partition[partitionId]*UnpartitionedMemoryIdx

woodsaj · 2019-03-08T10:45:21Z

because i feel like this could help a lot for multi tenant setups with many tenants:

it could, but the majority of MT clusters are single tenant.

It might still make sense to do, but it is not something that should be included in this PR

replay

LGTM

Dieterbe · 2019-03-11T23:48:53Z

@woodsaj at this point, would you recommend enabling the partitioning ? in which circumstances?

Dieterbe · 2019-03-12T00:03:52Z

idx/cassandra/cassandra.go

+	var defs []schema.MetricDefinition
+	var num int
+	for _, partition := range cluster.Manager.GetPartitions() {
+		defs = c.LoadPartitions([]int32{partition}, defs[:0], pre)


@woodsaj isn't this a bug? subsequent calls of this seem to overwrite previously loaded defs?

idx/memory/memory.go

woodsaj requested review from replay and robert-milan March 7, 2019 11:36

robert-milan approved these changes Mar 7, 2019

View reviewed changes