fetcher: add filter to remove old blocks if new overlapping blocks found #2071

khyatisoneji · 2020-01-28T18:41:07Z

Changes

Add Filter to filter out old blocks if new overlapping blocks found that fully submatch the old blocks

Please let me know if this is the right direction to go in terms of filtering out the old blocks

bwplotka

Nice! Good work, this looks like it will work... but only for basic case of blocks:

[A:A] [B:B] [C:C] [D:ABC]

This is great however if we have only compaction levels higher than 2 we can have e.g:

[H:ABC][I:DFG][J:ABCDEFG] which will be not detected here.

I would say even for development purposes having a table unit test is really helpful for yoursefl! In this case I would even start with unit test (Test driven development!). You specify use cases, you implement the code and verify in seconds ❤️ This is what I would do (:

For your information this is what Prometheus/TSDB is doing when compacting two blocks together, how it combines Sources: https://github.com/prometheus/prometheus/blob/dee6981a6c78dfc28d000f86e6a6639cc37150b3/tsdb/compact.go#L332

bwplotka · 2020-01-29T06:57:59Z

pkg/block/fetcher.go

+	return &CompactedBlockFilter{}
+}
+
+// Filter filters out blocks that filters out blocks that have newer block


Filter filters out blocks that filters out blocks? =D

Still not addressed ):

bwplotka · 2020-01-29T06:58:52Z

pkg/block/fetcher.go

@@ -410,3 +411,52 @@ func (f *LabelShardedMetaFilter) Filter(metas map[ulid.ULID]*metadata.Meta, sync
 		delete(metas, id)
 	}
 }
+
+// CompactedBlockFilter is a MetaFetcher filter that filters out blocks that have newer block


have newer block from two or more overlappings blocks?

What does it mean for block to have blocks? 🤔

Not addressed.

bwplotka · 2020-01-29T07:04:50Z

pkg/block/fetcher.go

+	for id, meta := range metas {
+		sources := make([]ulid.ULID, len(metas))
+		for _, sourceID := range meta.Compaction.Sources {
+			if sourceID == id {


Let's use sourceID.Compare(id) == 0 to be safe maybe (:

bwplotka · 2020-01-29T07:22:13Z

pkg/block/fetcher.go

+		if areSlicesEqual(sources, meta.Compaction.Sources) {
+			for _, sourceID := range meta.Compaction.Sources {
+				if sourceID != id {
+					delete(metas, sourceID)


Next to deleting, we probably want store the deleted metas and check if we are ready to delete those blocks from bucket (:

bwplotka

Nice work!

There are some things to improve, but overall good work!

I would focus on:

simplifying vertex, remove forest thing - it's just another vertex.
addVertex method name is not enough, it does a lot more than just add.
avoid shallow functions/methods
Remove repetitions from test cases
Be consistent and clear on test cases names

But apart from that this looks amazing! (:

I think once you will address those comments (Of course if you don't agree with anything, just tell me!), And we will be happy with functionality we can look on optimizing things. I can see at least 4 places where we can save some CPU and memory, BUT micro optimization should be done based on numbers, so we will start with creating some benchmark, and then optimize it (:

Do you want to pair program sometime this week on the go benchmark? @khyatisoneji

cmd/thanos/compact.go

bwplotka · 2020-02-02T15:08:29Z

cmd/thanos/compact.go

 	metaFetcher, err := block.NewMetaFetcher(logger, 32, bkt, "", extprom.WrapRegistererWithPrefix("thanos_", reg),
 		block.NewLabelShardedMetaFilter(relabelConfig).Filter,
 		(&consistencyDelayMetaFilter{logger: logger, consistencyDelay: consistencyDelay}).Filter,
+		compactedBlockFilter.Filter,


Wonder if it wouldn't be beter if the Syncer would have internal method Filter and DuplicatedBlocks()

Syncer is not used by store component, so we need to add the filter there explicitly then

pkg/block/fetcher.go

bwplotka · 2020-02-02T15:11:54Z

pkg/block/fetcher.go

+	return &CompactedBlockFilter{}
+}
+
+// Filter filters out blocks that filters out blocks that have newer block


Still not addressed ):

pkg/block/fetcher_test.go

bwplotka · 2020-02-02T20:11:28Z

pkg/block/fetcher_test.go

+			},
+		},
+		{
+			name: "3 non compacted metas and compacted meta of level 2 in bucket",


again, meta is never technically compacted -> we need to consistent and clear in naming here. Also compaction does not matter here: Just what constains same sources.

For example this case could be named: Block 4 contains 1, 2, 3

Also I believe :

ULID: ULID(4), Compaction: tsdb.BlockMetaCompaction{ Sources: []ulid.ULID{ULID(1), ULID(2), ULID(3), ULID(4)}, },

.. is wrong. This will never happen as 4 is compacted block, so it will be never part of sources I believe (That's why I linked the Prometheus code which does it, to double check) (:

pkg/block/fetcher_test.go

bwplotka · 2020-02-02T20:13:13Z

pkg/block/fetcher_test.go

+			},
+		},
+	} {
+		f := NewCompactedBlockFilter()


Can we add case with 2 blocks having exactly the same sources?

Yes, its added with test case name: two compacted blocks with same sources

bwplotka · 2020-02-02T20:15:11Z

pkg/block/forest.go

+		rootSources := metas[vertex.ID].Compaction.Sources
+		sources := metas[id].Compaction.Sources
+
+		if f.containsSources(rootSources, sources) && f.containsSources(sources, rootSources) { // a block exists with same sources.


This line is quite long so maybe we can put comment above? And make it a full sentence? (Capital first letter)

bwplotka · 2020-02-02T20:18:39Z

pkg/block/forest.go

+	for _, a := range s2 {
+		found := false
+		for _, e := range s1 {
+			if a.Compare(e) == 0 {


Nice. I think we can optimize a bit but this willl do for now (:

We can create some go benchmark for this in next PR 💪

khyatisoneji · 2020-02-03T11:32:07Z

@bwplotka I have addressed some concerns that you have made:

Removing forest and having only vertex
Name test cases with compacted blocks and not compacted metas.
Removed the function for expected metas, currently only comparing the slices.
Renamed ulidsToDelete with duplicateIds
Rename CompactedBlockFilter with DeduplicateFilter

I have one suggestion that we should address deletion in a separate PR. Let's narrow down scope of this PR to only filter out the duplicate blocks.
Deletion of the blocks, as you mentioned should be done with checks mentioned in the proposal, so adding them in a separate PR would be better.

bwplotka · 2020-02-03T12:50:51Z

I have one suggestion that we should address deletion in a separate PR. Let's narrow down scope of this PR to only filter out the duplicate blocks.
Deletion of the blocks, as you mentioned should be done with checks mentioned in the proposal, so adding them in a separate PR would be better.

Very good idea. But in this case we should not probably hook this filter yet in Compactor I think. We can hook in Store Gateway though.

bwplotka · 2020-02-03T12:55:57Z

pkg/block/fetcher.go

+// Filter filters out duplicate blocks that can be formed
+// from two or more overlapping blocks that fully submatches the source blocks of the older blocks.
+func (f *DeduplicateFilter) Filter(metas map[ulid.ULID]*metadata.Meta, synced GaugeLabeled, _ bool) {
+	forest := NewVertex(ulid.MustNew(uint64(0), nil), nil)


still forest name, maybe just root?

bwplotka · 2020-02-03T12:56:28Z

pkg/block/forest.go

+)
+
+// Vertex type implements a vertex of a Forest.
+type Vertex struct {


filename is still old (forest)

bwplotka · 2020-02-03T12:57:34Z

pkg/block/forest.go

+	return ulids
+}
+
+func (f *Vertex) add(id ulid.ULID, startVertex *Vertex, metas map[ulid.ULID]*metadata.Meta) bool {


add is still cryptic. At least can we have a comment what this method does? (: Because it's not just vertex - it's a vertex that

Also:

Suggested change

func (f *Vertex) add(id ulid.ULID, startVertex *Vertex, metas map[ulid.ULID]*metadata.Meta) bool {

func (v *Vertex) add(id ulid.ULID, startVertex *Vertex, metas map[ulid.ULID]*metadata.Meta) bool {

bwplotka · 2020-02-03T12:58:24Z

pkg/block/forest.go

+	return f.add(id, rootVertex, metas)
+}
+
+func (f *Vertex) containsSources(rootSources, sources []ulid.ULID) bool {


Does it have to be method? Also, I don't get why we need this at all this method does nothing ): It might too shallow

bwplotka

Nice! Let's move add method maybe as suggested and fix test suggestions and we are good to start some benchmarking! What do you think? (: In next PRs obviously... Exciting!

pkg/block/fetcher.go

bwplotka · 2020-02-03T18:45:02Z

pkg/block/fetcher.go

+	})
+
+	for _, meta := range metaSlice {
+		root.AddMeta(meta.ULID, metas)


Still, I would vote to move it as a separate function (:

bwplotka · 2020-02-03T18:45:14Z

pkg/block/fetcher_test.go

+		{
+			name: "two blocks with same sources",
+			input: map[ulid.ULID]*metadata.Meta{
+				ULID(3): {


not addressed

pkg/block/fetcher_test.go

bwplotka · 2020-02-03T18:50:25Z

pkg/block/node.go

+	"github.com/thanos-io/thanos/pkg/block/metadata"
+)
+
+// Node type implements a node of a Forest.


forest still ):

Suggestions? Node of a tree?

Aren't both test cases relevant? The first one is for case where two blocks have same sources and second one is where the blocks have overlapping sources.

bwplotka · 2020-02-03T19:26:00Z

pkg/block/node.go

+	return ulids
+}
+
+func (n *Node) add(id ulid.ULID, startNode *Node, metas map[ulid.ULID]*metadata.Meta, areEqual, isChildNode compareNodes) bool {


Ok, I said like 4 times to move this from node and it's still there and you did not explain why you are against this (:

Maybe I can send PR to your PR to show you how it can be done and show adventages? (:

The main problem is that this is just a node

It's not hierarchicalBlocksNode or anything like this. Method is also just add which suggests nothing is done expect just adding a node to the tree ... somewhere. This is might be not clear for readers of this code so we need to improve this.

Second argument why I believe it would nice to move add to separate function is extensibility. What if we would like to use our tree with meta.jsons data struct for other algorithms? We can't because add has certain logic baked in.

bwplotka · 2020-02-03T21:44:20Z

pkg/block/fetcher_test.go

+func TestDeduplicateFilter_Filter(t *testing.T) {
+	for _, tcase := range []struct {
+		name     string
+		input    map[ulid.ULID]tsdb.BlockMetaCompaction


Maybe just map[ulid.ULID][]ulid.ULID?

bwplotka

Awesome! I think we need to fix todo and add more test cases and we are good to merge and play with some benchmarking ❤️

Thanks!

bwplotka · 2020-02-05T11:24:53Z

pkg/block/fetcher.go

+		if metas[id] != nil {
+			f.DuplicateIDs = append(f.DuplicateIDs, id)
+		}
+		// TODO(khyati): Increment synced metrics with proper state.


bwplotka · 2020-02-05T11:26:11Z

pkg/block/fetcher_test.go

+		{
+			name: "compacted block with sources in bucket",
+			input: map[ulid.ULID][]ulid.ULID{
+				ULID(4): []ulid.ULID{ULID(1), ULID(2), ULID(3)},


Let's put other blocks here that we don't expect to be compacted

bwplotka · 2020-02-05T11:26:17Z

pkg/block/fetcher_test.go

+		{
+			name: "two compacted blocks with same sources",
+			input: map[ulid.ULID][]ulid.ULID{
+				ULID(3): []ulid.ULID{ULID(1), ULID(2)},


ditto, let's mix it

bwplotka · 2020-02-05T11:26:45Z

pkg/block/fetcher_test.go

+		{
+			name: "two compacted blocks with overlapping sources",
+			input: map[ulid.ULID][]ulid.ULID{
+				ULID(4): []ulid.ULID{ULID(1), ULID(2)},


bwplotka · 2020-02-05T11:27:21Z

pkg/block/fetcher_test.go

+			name: "two compacted blocks with overlapping sources",
+			input: map[ulid.ULID][]ulid.ULID{
+				ULID(4): []ulid.ULID{ULID(1), ULID(2)},
+				ULID(5): []ulid.ULID{ULID(1), ULID(2), ULID(3)},


Can we reorder ULIDs in Source to check if that will work?

bwplotka · 2020-02-05T11:28:08Z

pkg/block/fetcher_test.go

+		{
+			name: "3 compacted blocks of level 2 and one compacted block of level 3 in bucket",
+			input: map[ulid.ULID][]ulid.ULID{
+				ULID(10): []ulid.ULID{ULID(1), ULID(2), ULID(3)},


ditto, let's mix with some block that should stay

bwplotka · 2020-02-05T11:28:56Z

pkg/block/fetcher_test.go

+			input: map[ulid.ULID][]ulid.ULID{
+				ULID(1): []ulid.ULID{ULID(1)},
+				ULID(5): []ulid.ULID{ULID(1), ULID(2)},
+				ULID(6): []ulid.ULID{ULID(1), ULID(2), ULID(3), ULID(4)},


Let's reorder those a bit to make it more tricky. Yes we put it in map anyway, but sometimes map tends to be.. accidently ordered (:

bwplotka · 2020-02-05T11:30:17Z

pkg/block/fetcher_test.go

+	} {
+		f := NewDeduplicateFilter()
+		if ok := t.Run(tcase.name, func(t *testing.T) {
+			synced := prometheus.NewGaugeVec(prometheus.GaugeOpts{}, []string{"state"})


We can assert on this metric!

See our talk: https://are-you-testing-your-observability.now.sh/

Click right until 26 slide and check it out (:

bwplotka · 2020-02-05T11:32:32Z

pkg/block/fetcher_test.go

+				ULID(5): []ulid.ULID{ULID(1), ULID(2)},
+				ULID(6): []ulid.ULID{ULID(1), ULID(2), ULID(3), ULID(4)},
+				ULID(7): []ulid.ULID{ULID(1), ULID(2), ULID(3)},
+				ULID(8): []ulid.ULID{ULID(1), ULID(2), ULID(3), ULID(4)},


Let's also swap ULID(7) to ULID(8) (: Make it more tricky!

bwplotka · 2020-02-05T11:32:43Z

pkg/block/fetcher_test.go

+				ULID(7): []ulid.ULID{ULID(1), ULID(2), ULID(3)},
+				ULID(8): []ulid.ULID{ULID(1), ULID(2), ULID(3), ULID(4)},
+			},
+			expected: []ulid.ULID{


Can we add one more case?

The case would be something like

10 containing 1,2,6,7
11 containing 1,2,6,8
(:
And maybe:
10 containing 1,2,6,7
11 containing 1,2,3

khyatisoneji · 2020-02-05T18:14:32Z

Added the requested changes:

implemented todo to add sync metric for filtered duplicates
Reordered ulids in test
Mixed compacted and non compacted ulids in test
Added requested test additions
added test to test metric for filtered duplicates.

Signed-off-by: khyatisoneji <[email protected]>

bwplotka

Let's go...!

Solid work, thanks! 👍

bwplotka · 2020-02-05T22:33:27Z

pkg/block/fetcher_test.go

+}
+
+func compareSliceWithMapKeys(tb testing.TB, m map[ulid.ULID]*metadata.Meta, s []ulid.ULID) {
+	_, file, line, _ := runtime.Caller(1)


Wow this is epic! Probably bit over-engineered, but... thanks! (:

khyatisoneji force-pushed the old_block_filter branch from 21f3fc4 to 2059766 Compare January 28, 2020 18:41

GiedriusS requested review from bwplotka and GiedriusS January 28, 2020 18:58

bwplotka reviewed Jan 29, 2020

View reviewed changes

khyatisoneji force-pushed the old_block_filter branch 6 times, most recently from bc34b89 to 31c1507 Compare February 1, 2020 15:18

bwplotka reviewed Feb 2, 2020

View reviewed changes

khyatisoneji force-pushed the old_block_filter branch 3 times, most recently from efa1d15 to 57a001d Compare February 3, 2020 11:26

bwplotka reviewed Feb 3, 2020

View reviewed changes

khyatisoneji force-pushed the old_block_filter branch 5 times, most recently from 32c58b6 to 4b69821 Compare February 3, 2020 18:18

bwplotka approved these changes Feb 3, 2020

View reviewed changes

khyatisoneji force-pushed the old_block_filter branch 2 times, most recently from ce6ee20 to e5b6eb4 Compare February 3, 2020 19:13

bwplotka reviewed Feb 3, 2020

View reviewed changes

khyatisoneji force-pushed the old_block_filter branch from e5b6eb4 to 8133cbe Compare February 3, 2020 20:06

bwplotka reviewed Feb 3, 2020

View reviewed changes

khyatisoneji force-pushed the old_block_filter branch 2 times, most recently from 90d8998 to 4a96f13 Compare February 4, 2020 12:00

khyatisoneji force-pushed the old_block_filter branch from 0b57901 to 03e85d7 Compare February 5, 2020 09:29

bwplotka approved these changes Feb 5, 2020

View reviewed changes

khyatisoneji force-pushed the old_block_filter branch from 03e85d7 to 53507c0 Compare February 5, 2020 18:11

khyatisoneji force-pushed the old_block_filter branch from 53507c0 to 3e0f88e Compare February 5, 2020 18:16

fetcher: add filter to remove old blocks if new overlapping blocks found

025b327

Signed-off-by: khyatisoneji <[email protected]>

khyatisoneji force-pushed the old_block_filter branch from 3e0f88e to 025b327 Compare February 5, 2020 18:38

bwplotka approved these changes Feb 5, 2020

View reviewed changes

bwplotka merged commit 6948d5b into thanos-io:master Feb 5, 2020

	func (f Vertex) add(id ulid.ULID, startVertex Vertex, metas map[ulid.ULID]*metadata.Meta) bool {
	func (v Vertex) add(id ulid.ULID, startVertex Vertex, metas map[ulid.ULID]*metadata.Meta) bool {

fetcher: add filter to remove old blocks if new overlapping blocks found #2071

fetcher: add filter to remove old blocks if new overlapping blocks found #2071

Conversation

khyatisoneji commented Jan 28, 2020

Changes

bwplotka 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

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

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

khyatisoneji commented Feb 3, 2020

bwplotka commented Feb 3, 2020

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

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

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

bwplotka 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

khyatisoneji commented Feb 5, 2020

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a comment •

edited

Loading

bwplotka left a comment •

edited

Loading