remove branch vs leaf node restriction in index #490

woodsaj · 2017-01-20T17:32:32Z

fixes #392

woodsaj · 2017-01-20T17:33:03Z

This is mostly done, just need to add unit tests to verify that the index works for mixed branch/leaf nodes

woodsaj · 2017-01-20T18:00:06Z

no significant change in performance of the index.

old

anthony:~/go/src/github.com/raintank/metrictank/idx/memory$ go test -run NONE -bench=. -benchmem 
BenchmarkFind-4              	   10000	    184041 ns/op	   51589 B/op	     877 allocs/op
BenchmarkConcurrent4Find-4   	   20000	    151151 ns/op	   51600 B/op	     877 allocs/op
BenchmarkConcurrent8Find-4   	   20000	    154791 ns/op	   51576 B/op	     877 allocs/op
BenchmarkIndexing-4          	  500000	      3658 ns/op	    1155 B/op	      22 allocs/op
PASS

new

anthony:~/go/src/github.com/raintank/metrictank/idx/memory$ go test -run NONE -bench=. -benchmem 
BenchmarkFind-4              	   10000	    196579 ns/op	   53480 B/op	     877 allocs/op
BenchmarkConcurrent4Find-4   	   20000	    183117 ns/op	   53487 B/op	     877 allocs/op
BenchmarkConcurrent8Find-4   	   20000	    155884 ns/op	   53451 B/op	     876 allocs/op
BenchmarkIndexing-4          	  500000	      3754 ns/op	    1187 B/op	      22 allocs/op
PASS

Dieterbe · 2017-01-23T09:31:33Z

idx/memory/memory_test.go

@@ -96,6 +98,7 @@ func TestGetAddKey(t *testing.T) {
 }

 func TestFind(t *testing.T) {
+	log.NewLogger(0, "console", fmt.Sprintf(`{"level": %d, "formatting":false}`, 1))


what does this do? i don't see any log calls in any of the tests in idx/memory or is this to make the log calls in memory.go work?

this is just left over from me debugging (it makes the log calls in memory.go work). It should be removed.

Dieterbe · 2017-01-23T09:35:50Z

idx/memory/memory.go

+	Children    []string
+	Defs        []string
+	Leaf        bool
+	HasChildren bool
 }


can you add a comment to the Node type here as well as the one in idx.go , explaining their respective purpose. for the reader (at least for me) it's confusing why there are two types, how and why they differ.

I am considering changing the struct to just

type Node struct { Path string Children []string Defs []string }

And the node is a leaf if len(n.Defs) > 0 and the node HasChildren if len(n.Children) > 0

Thoughts?

sounds fine by me. and those two concepts are independent from each other right?

yes. a node can be a leaf node or a branch node (hasChildren) or both.

can you add a comment to the Node type here as well as the one in idx.go , explaining their respective purpose. for the reader (at least for me) it's confusing why there are two types, how and why they differ.

woodsaj · 2017-01-30T17:17:55Z

lets get PR #504 merged, then i will tackle this again.

woodsaj · 2017-02-02T07:05:45Z

This is now ready for review.

Unfortunately, the Grafana query editor is unable to view series that are a descendant of a leaf node as graphite still does not correctly support this.

When you have a series called "foo.bar" and a second series called "foo.bar.baz", carbon will write a whisper file for each. However the graphite "/metrics/find?" query only supports sending back that a node is a leaf or a branch and not both due to:

https://github.com/graphite-project/graphite-web/blob/master/webapp/graphite/metrics/views.py#L270-L283

graphite-api does the exact same thing

However, if you set the Grafana queryEditor to raw mode, you can add the full series path and it will render.

When we add graphite query handling directly to MT, we can fix this limitation.

replay · 2017-02-02T14:27:40Z

idx/memory/memory.go

@@ -242,17 +237,17 @@ func (m *MemoryIdx) add(def *schema.MetricDefinition) error {
 		log.Debug("memory-idx: creating branch %s with child %s", branch, nodes[i])


After this loop startPos is not used anymore. So instead of declaring i and assigning the value of startPos to it, we could also just use startPos in place of i here

startPos is used after this loop and I dont see how i can maintain the same logic when removing i

hmm... ok.
Basically my idea was this, but I must be missing where else it's used...

// Add missing branch nodes for ; startPos < len(nodes); startPos++ { branch := strings.Join(nodes[0:startPos], ".") log.Debug("memory-idx: creating branch %s with child %s", branch, nodes[startPos]) tree.Items[branch] = &Node{ Path: branch, Children: []string{nodes[startPos]}, Defs: make([]string, 0), } } // Add leaf node log.Debug("memory-idx: creating leaf %s", path) tree.Items[path] = &Node{ Path: path, Children: []string{}, Defs: []string{def.Id}, } m.DefById[def.Id] = def statAddOk.Inc() return nil }

That definitely wont work.

The purpose of this loop is to search backwards down the tree until we find a branch that exists.
then from https://github.com/raintank/metrictank/blob/9b2623b29398feb54795eb4bf0a9ecc42f544ba0/idx/memory/memory.go#L231-L243
we walk back up the tree creating all of the needed nodes.

ahh, nevermind i was looking at the wrong loop. Your suggestion will work

are we talking about the same loop? actually that's really not important enough to spend time arguing, but i think according to your description you're referring to the previous loop.

replay

Looks good to me... That's some impressive data structure management

woodsaj added this to the hosted-metrics-alpha milestone Jan 20, 2017

woodsaj force-pushed the removeBranchLeafRestriction branch from 38236ee to e6dbcfa Compare January 20, 2017 18:08

Dieterbe reviewed Jan 23, 2017

View reviewed changes

woodsaj force-pushed the removeBranchLeafRestriction branch from e6dbcfa to 9b2623b Compare February 2, 2017 06:23

woodsaj changed the title ~~WIP: remove branch vs leaf node restriction in index~~ remove branch vs leaf node restriction in index Feb 2, 2017

woodsaj requested a review from replay February 2, 2017 06:54

Dieterbe self-assigned this Feb 2, 2017

replay reviewed Feb 2, 2017

View reviewed changes

replay approved these changes Feb 2, 2017

View reviewed changes

remove branch vs leaf node restriction in index

008737c

woodsaj force-pushed the removeBranchLeafRestriction branch from 9b2623b to 008737c Compare February 3, 2017 13:38

woodsaj merged commit 89e6f46 into master Feb 5, 2017

woodsaj deleted the removeBranchLeafRestriction branch February 5, 2017 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove branch vs leaf node restriction in index #490

remove branch vs leaf node restriction in index #490

woodsaj commented Jan 20, 2017

woodsaj commented Jan 20, 2017

woodsaj commented Jan 20, 2017 •

edited by Dieterbe

Loading

Dieterbe Jan 23, 2017

woodsaj Jan 23, 2017

Dieterbe Jan 23, 2017 •

edited

Loading

woodsaj Jan 23, 2017 •

edited

Loading

Dieterbe Jan 23, 2017

woodsaj Jan 23, 2017

Dieterbe Feb 2, 2017

woodsaj commented Jan 30, 2017

woodsaj commented Feb 2, 2017

replay Feb 2, 2017

woodsaj Feb 2, 2017

replay Feb 2, 2017

woodsaj Feb 2, 2017

woodsaj Feb 2, 2017

replay Feb 2, 2017

replay left a comment

		@@ -242,17 +237,17 @@ func (m MemoryIdx) add(def schema.MetricDefinition) error {
		log.Debug("memory-idx: creating branch %s with child %s", branch, nodes[i])

remove branch vs leaf node restriction in index #490

remove branch vs leaf node restriction in index #490

Conversation

woodsaj commented Jan 20, 2017

woodsaj commented Jan 20, 2017

woodsaj commented Jan 20, 2017 • edited by Dieterbe Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dieterbe Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

woodsaj Jan 23, 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

woodsaj commented Jan 30, 2017

woodsaj commented Feb 2, 2017

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 left a comment

Choose a reason for hiding this comment

woodsaj commented Jan 20, 2017 •

edited by Dieterbe

Loading

Dieterbe Jan 23, 2017 •

edited

Loading

woodsaj Jan 23, 2017 •

edited

Loading