[WIP] compact: Add vertical-deduplication for replica-labels as MetaFetcher filter #2217

metalmatze · 2020-03-05T13:21:36Z

I added CHANGELOG entry for this change.
Change is not relevant to the end-user.

Need to create the PR first to get a number 😁

Changes

Add MetaFetcher filter called ReplicaLabelsFilter in: c6828f9
Other commits are to add e2e tests.

Verification

This is the reason why it's still WIP. I want to finish the end to end tests, but I get minio errors. I'm not able to fix it right now.

compact_test.go:60: unexpected error: upload file /tmp/e2e_integration_test767365421/tmp/01E2NBJ5Q70000000000000000/chunks/000001 as 01E2NBJ5Q70000000000000000/chunks/000001: upload s3 object: Get "http://e2e_test_compact-minio-80/thanos/?location=": dial tcp: lookup e2e_test_compact-minio-80: no such host

/cc @brancz @bwplotka @squat

Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>

bwplotka

Nice!

Super close, some comments we did together with @kakkoyun. He is happy to continue your PR.

bwplotka · 2020-03-10T13:16:25Z

cmd/thanos/compact.go

@@ -121,6 +121,9 @@ func registerCompact(m map[string]setupFunc, app *kingpin.Application) {
 	compactionConcurrency := cmd.Flag("compact.concurrency", "Number of goroutines to use when compacting groups.").
 		Default("1").Int()

+	dedupReplicaLabels := cmd.Flag("offline-deduplication.replica-labels", "Label to treat as a replica indicator of blocks that can be deduplicated. This will merge multiple replica blocks into one. This process is irrevertable. Experminteal").


Suggested change

dedupReplicaLabels := cmd.Flag("offline-deduplication.replica-labels", "Label to treat as a replica indicator of blocks that can be deduplicated. This will merge multiple replica blocks into one. This process is irrevertable. Experminteal").

dedupReplicaLabels := cmd.Flag("deduplication.replica-label", "Label to treat as a replica indicator of blocks that can be deduplicated. This will merge multiple replica blocks into one (repeated flag). This process is irreversible. Experimental").

Let's make sure it's documented that it's ONLY vertical for now and what it means.

bwplotka · 2020-03-10T13:16:42Z

cmd/thanos/compact.go

@@ -121,6 +121,9 @@ func registerCompact(m map[string]setupFunc, app *kingpin.Application) {
 	compactionConcurrency := cmd.Flag("compact.concurrency", "Number of goroutines to use when compacting groups.").
 		Default("1").Int()

+	dedupReplicaLabels := cmd.Flag("offline-deduplication.replica-labels", "Label to treat as a replica indicator of blocks that can be deduplicated. This will merge multiple replica blocks into one. This process is irrevertable. Experminteal").
+		Hidden().String()


Suggested change

Hidden().String()

Hidden().Strings()

Let's add comment to Hidden why it's hidden

bwplotka · 2020-03-10T13:17:22Z

cmd/thanos/compact.go

@@ -242,10 +247,13 @@ func runCompact(

 	duplicateBlocksFilter := block.NewDeduplicateFilter()
 	prometheusRegisterer := extprom.WrapRegistererWithPrefix("thanos_", reg)
+	replicaLabelFilter := block.ReplicaLabelsFilter{ReplicaLabels: strings.Split(dedupReplicaLabels, ",")}


Suggested change

replicaLabelFilter := block.ReplicaLabelsFilter{ReplicaLabels: strings.Split(dedupReplicaLabels, ",")}

replicaLabelFilter := block.ReplicaLabelFilter{ReplicaLabels: dedupReplicaLabels}

bwplotka · 2020-03-10T13:18:40Z

cmd/thanos/compact.go

@@ -242,10 +247,13 @@ func runCompact(

 	duplicateBlocksFilter := block.NewDeduplicateFilter()
 	prometheusRegisterer := extprom.WrapRegistererWithPrefix("thanos_", reg)
+	replicaLabelFilter := block.ReplicaLabelsFilter{ReplicaLabels: strings.Split(dedupReplicaLabels, ",")}


Can we inline this?

bwplotka · 2020-03-10T13:20:20Z

pkg/block/fetcher.go

@@ -533,6 +534,23 @@ func contains(s1 []ulid.ULID, s2 []ulid.ULID) bool {
 	return true
 }

+type ReplicaLabelsFilter struct {


Suggested change

type ReplicaLabelsFilter struct {

type ReplicaLabelConstructingFakeMetaFilter struct {

;p

Actually as @kakkoyun found it's not a filter, so maybe different name

"Modifier"?

bwplotka · 2020-03-10T13:29:51Z

cmd/thanos/compact.go

@@ -242,10 +247,13 @@ func runCompact(

 	duplicateBlocksFilter := block.NewDeduplicateFilter()


Somewhere we need to enable vertical compaction flag IF replica flag is enabled on LeveledCompactor

bwplotka · 2020-03-10T13:30:42Z

test/e2e/compact_test.go

+	dir := filepath.Join(s.SharedDir(), "tmp")
+
+	now := time.Now()
+	id1, err := e2eutil.CreateBlockWithBlockDelay(ctx, dir, series, 10, timestamp.FromTime(now), timestamp.FromTime(now.Add(2*time.Hour)), 30*time.Minute, extLset, 0)


We don''t need or want delay I think

bwplotka · 2020-03-10T13:31:01Z

test/e2e/compact_test.go

+		Insecure:  true,
+	}
+
+	series := []labels.Labels{labels.FromStrings("a", "1", "b", "2")}


Let's maybe put block creation to separate function, because we need to create tons of them for the tests

Potentiall would be nice to have a table test for different tests

Offline Dedup

Vertical dedup (let's focus in this)

Downs

Compacti

retention

etc

bwplotka · 2020-03-10T13:33:44Z

test/e2e/compact_test.go

+	})
+	testutil.Ok(t, err)
+	testutil.Ok(t, s.StartAndWaitReady(compact))
+


On each run of table test, we need to create some blocks, then assert something.

Open question: In table test: Should we clean up the bucket or clean up + restart compactor.

bwplotka · 2020-03-10T13:34:32Z

test/e2e/e2ethanos/services.go

+		return nil, errors.Wrapf(err, "generate compact config file: %v", bucketConfig)
+	}
+
+	fmt.Println(string(bktConfigBytes))


kakkoyun · 2020-03-10T15:34:40Z

@bwplotka @squat As we have discussed offline, picking up from where @metalmatze left off in #2250.

metalmatze added 2 commits March 5, 2020 14:22

Create ReplicaLabelsFilter to allow for offline deduplication

c6828f9

Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>

Start adding a e2e test for offline-deduplication with Thanos compact

e24a283

Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>

metalmatze force-pushed the offline-deduplication.replica-labels branch from ec2836a to e24a283 Compare March 5, 2020 13:22

bwplotka changed the title ~~[WIP] compact: Add offline-deduplication for replica-labels as MetaFetcher filter~~ [WIP] compact: Add vertical-deduplication for replica-labels as MetaFetcher filter Mar 10, 2020

bwplotka requested changes Mar 10, 2020

View reviewed changes

kakkoyun mentioned this pull request Mar 10, 2020

compactor: Add ReplicaLabelRemover as MetaFetcher filter to enable offline vertical compaction/deduplication for replicated data #2250

Merged

2 tasks

metalmatze closed this Mar 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] compact: Add vertical-deduplication for replica-labels as MetaFetcher filter #2217

[WIP] compact: Add vertical-deduplication for replica-labels as MetaFetcher filter #2217

metalmatze commented Mar 5, 2020 •

edited

Loading

bwplotka left a comment

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

bwplotka Mar 10, 2020

kakkoyun commented Mar 10, 2020 •

edited

Loading

	dedupReplicaLabels := cmd.Flag("offline-deduplication.replica-labels", "Label to treat as a replica indicator of blocks that can be deduplicated. This will merge multiple replica blocks into one. This process is irrevertable. Experminteal").
	dedupReplicaLabels := cmd.Flag("deduplication.replica-label", "Label to treat as a replica indicator of blocks that can be deduplicated. This will merge multiple replica blocks into one (repeated flag). This process is irreversible. Experimental").

	replicaLabelFilter := block.ReplicaLabelsFilter{ReplicaLabels: strings.Split(dedupReplicaLabels, ",")}
	replicaLabelFilter := block.ReplicaLabelFilter{ReplicaLabels: dedupReplicaLabels}

	type ReplicaLabelsFilter struct {
	type ReplicaLabelConstructingFakeMetaFilter struct {

		@@ -242,10 +247,13 @@ func runCompact(

		duplicateBlocksFilter := block.NewDeduplicateFilter()

[WIP] compact: Add vertical-deduplication for replica-labels as MetaFetcher filter #2217

[WIP] compact: Add vertical-deduplication for replica-labels as MetaFetcher filter #2217

Conversation

metalmatze commented Mar 5, 2020 • edited Loading

Changes

Verification

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

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

kakkoyun commented Mar 10, 2020 • edited Loading

metalmatze commented Mar 5, 2020 •

edited

Loading

kakkoyun commented Mar 10, 2020 •

edited

Loading