Add support to horizontally scale blocks compactor #2113

pracucci · 2020-02-11T14:53:03Z

What this PR does:
This PR introduces the support to horizontally scale the compactor, when using the experimental blocks storage.

I've also took the change to remove ingester references from the ring HTTP page, given it's now used by multiple components (ingesters, ruler, compactor). The refactoring done here should be safe, given we've recently removed support for denormalised tokens.

Which issue(s) this PR fixes:
N/A

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

thorfour

Great work!

I guess my only question is during a topology change is there not a way that two compactors can run on the same user? Ex. first compactor comes up, adds itself to the ring and starts compacting user 0. Next compactor starts up adds itself to the ring, and it now owns user 0 and will start compaction on that user, resulting int two compactors working on the same user.

pkg/compactor/compactor.go

thorfour · 2020-02-11T16:31:55Z

pkg/compactor/compactor.go

+	for {
+		// Check if the ingester is ACTIVE in the ring and our ring client
+		// has detected it.
+		if rs, err := c.ring.GetAll(); err == nil {


Is this check necessary? Won't GetState only return ACTIVE if it's in the ring?

I'm not sure I'm getting your question. Here we need to check if the ring client (not the lifecycler) has detected the update (made by the lifecycler).

Disregard me, I need to go read the ring/lifecycler code again :)

khaines

This is a great feature to add! The PR also LGTM

pracucci · 2020-02-11T16:58:52Z

I guess my only question is during a topology change is there not a way that two compactors can run on the same user? Ex. first compactor comes up, adds itself to the ring and starts compacting user 0. Next compactor starts up adds itself to the ring, and it now owns user 0 and will start compaction on that user, resulting int two compactors working on the same user.

@thorfour Definitely. This can happen "by design". After looking at the bucket compactor logic, doing some manual experiments and talking to Bartek (back in December) this looked to be not a real issue. If the same blocks for the same tenant are simultaneously compacted by two instances, the next compactor run will get rid of the extra blocks created due to simultaneous runs (at this point only 1 of the two compactors will run the next compaction).

thorfour

LGTM awesome work, thanks for clarifying what happens when two compactors work on the same user 👍

jtlisi

LGTM

pstibrany

Overall PR looks good to me, but I'm concerned about resharding. What if one compactor is working on a user, and at the same time, new compactor is added – and suddenly it's new compactor who owns user 1. Does first compactor detect and cancels the compaction? Or does it not upload compacted blocks in such case?

Update: I see this has been addressed by other comments already.

CHANGELOG.md

pkg/ring/http.go

pkg/compactor/compactor_ring.go

pstibrany · 2020-02-12T11:00:14Z

pkg/ring/http.go

 		ShowTokens bool
 	}{
 		Ingesters:  ingesters,
 		Now:        time.Now(),
-		Ring:       ringDescString,


I have found it useful in the past to be able to see original ring descriptor, as stored in KV store. Can we still expose it, similarly how we hide tokens by default?

[Moved comment to relevant line]

Can we still expose it [...] ?

I can even rollback to the previous logic, but was containing the "ingester" naming which is quite confusing to users. Which information would you like to see? I may add it without using the marshalling of the entire RingDesc.

Well, it was useful when I introduced new ingester state, which was unknown to previous versions, but I could see on this page that it was actually received correctly. In general, I want to see all fields of the protobuf message exactly as they are stored in KV.

No need to rollback, your change is easier to read when one is interested in tokens only. My use-case is probably very rare [even more than checking tokens, which is already very rare].

pracucci · 2020-02-12T11:18:18Z

Overall PR looks good to me, but I'm concerned about resharding. What if one compactor is working on a user, and at the same time, new compactor is added – and suddenly it's new compactor who owns user 1. Does first compactor detect and cancels the compaction? Or does it not upload compacted blocks in such case?

@pstibrany It doesn't detect or cancel the on-going compaction. There's no strong guarantee two replicas will not compact the same blocks. My take is this one:
#2113 (comment)

Signed-off-by: Marco Pracucci <[email protected]>

pracucci · 2020-02-12T14:41:01Z

@pstibrany I should have addressed your comments. Do you have any other feedback?

pkg/ring/http.go

…ing page Signed-off-by: Marco Pracucci <[email protected]>

pstibrany

I should have addressed your comments. Do you have any other feedback?

Thanks Marco!

pracucci force-pushed the horizontally-scale-compactor branch from 796fa14 to 8045fcb Compare February 11, 2020 15:07

thorfour reviewed Feb 11, 2020

View reviewed changes

khaines approved these changes Feb 11, 2020

View reviewed changes

thorfour approved these changes Feb 11, 2020

View reviewed changes

jtlisi approved these changes Feb 12, 2020

View reviewed changes

pstibrany reviewed Feb 12, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/ring/http.go Outdated Show resolved Hide resolved

pkg/compactor/compactor_ring.go Show resolved Hide resolved

pstibrany reviewed Feb 12, 2020

View reviewed changes

pracucci added 7 commits February 12, 2020 15:20

Added sharding support to compactor

9cd50aa

Signed-off-by: Marco Pracucci <[email protected]>

Added compactor sharding tests

4832626

Signed-off-by: Marco Pracucci <[email protected]>

Added documentation

3fe9ef8

Signed-off-by: Marco Pracucci <[email protected]>

Updated changelog

2f2abf0

Signed-off-by: Marco Pracucci <[email protected]>

Fixed linter

652d51f

Signed-off-by: Marco Pracucci <[email protected]>

Fixed changelog

c7be914

Signed-off-by: Marco Pracucci <[email protected]>

Display tokens in a compact way in the ring page

5f3566c

Signed-off-by: Marco Pracucci <[email protected]>

pracucci force-pushed the horizontally-scale-compactor branch from a0f3077 to 5f3566c Compare February 12, 2020 14:42

pstibrany reviewed Feb 12, 2020

View reviewed changes

pkg/ring/http.go Outdated Show resolved Hide resolved

Write tokens in separate lines in the source code when building the r…

4e82ebe

…ing page Signed-off-by: Marco Pracucci <[email protected]>

pstibrany approved these changes Feb 12, 2020

View reviewed changes

pracucci merged commit 58ef607 into cortexproject:master Feb 12, 2020

pracucci deleted the horizontally-scale-compactor branch February 12, 2020 15:25

pracucci mentioned this pull request Feb 12, 2020

[Experimental] Memberlist singleton and support for multiple codecs #2016

Merged

3 tasks

Add support to horizontally scale blocks compactor #2113

Add support to horizontally scale blocks compactor #2113

Uh oh!

Conversation

pracucci commented Feb 11, 2020

Uh oh!

thorfour left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thorfour Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

pracucci Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

thorfour Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

khaines left a comment

Choose a reason for hiding this comment

Uh oh!

pracucci commented Feb 11, 2020

Uh oh!

thorfour left a comment

Choose a reason for hiding this comment

Uh oh!

jtlisi left a comment

Choose a reason for hiding this comment

Uh oh!

pstibrany left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pstibrany Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

pracucci Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

pstibrany Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

pracucci commented Feb 12, 2020

Uh oh!

pracucci commented Feb 12, 2020

Uh oh!

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pstibrany left a comment •

edited

Loading