Skip to content

Conversation

roystchiang
Copy link
Contributor

What this PR does:

Add proposal for parallel compaction by time interval

Which issue(s) this PR fixes:
Proposal to work on a fix for #3753

Checklist

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

@roystchiang roystchiang force-pushed the compactor-proposal branch from d94b281 to b386d7b Compare June 9, 2021 19:50
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good submission; I had some questions to clarify

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't follow this. What sort of corruption are you thinking about?
Is it related to this? thanos-io/thanos#4046

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of #3569

I suppose it's not exactly a corruption, but something that prevents compactor from compacting currently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I fully understand the proposed approach here. You can still compact all non-corrupted blocks together and just leave out of the compaction the corrupted block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not want to introduce a different behavior from what is currently happening. I wanted the proposal to be scoped to introducing horizontal scaling, and we can work on skipping bad blocks at a later stage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That idea seems fine. The amount of detail in the example seems overkill to make the point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That Thanos PR seems to be stalled; looks like Bartek called for a redesign.
The Google doc is just laying out the goals; it just says "TBD" for what would actually be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it still makes sense to reference it given that it is not really actively being developed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, always good to see where others have been before.
I guess I mostly wanted to clarify that this isn't something we can build on directly.

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this proposal! The overall design makes sense to me. I left few comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I fully understand the proposed approach here. You can still compact all non-corrupted blocks together and just leave out of the compaction the corrupted block.

This was referenced Jun 24, 2021
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some further comments, but they can be resolved later; happy to accept this proposal as it stands to unblock work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That idea seems fine. The amount of detail in the example seems overkill to make the point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, always good to see where others have been before.
I guess I mostly wanted to clarify that this isn't something we can build on directly.

---

## Introduction
As a part of pushing Cortex’s scaling capability at AWS, we have done performance testing with Cortex and found the compactor to be one of the main limiting factors for higher active timeseries limit per tenant. The documentation [Compactor](https://cortexmetrics.io/docs/blocks-storage/compactor/#how-compaction-works) describes the responsibilities of a compactor, and this proposal focuses on the limitations of the current compactor architecture. In the current architecture, compactor has simple sharding, meaning that a single tenant is sharded to a single compactor. In addition, a compactor handles compaction groups of a single tenant iteratively, meaning that blocks belonging non-overlapping times are not compacted in parallel.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"compaction groups" is not defined before use, either in this document or at the referenced link.
Possibly it could go in https://github.com/cortexproject/cortex/blob/master/docs/guides/glossary.md

As a part of pushing Cortex’s scaling capability at AWS, we have done performance testing with Cortex and found the compactor to be one of the main limiting factors for higher active timeseries limit per tenant. The documentation [Compactor](https://cortexmetrics.io/docs/blocks-storage/compactor/#how-compaction-works) describes the responsibilities of a compactor, and this proposal focuses on the limitations of the current compactor architecture. In the current architecture, compactor has simple sharding, meaning that a single tenant is sharded to a single compactor. In addition, a compactor handles compaction groups of a single tenant iteratively, meaning that blocks belonging non-overlapping times are not compacted in parallel.

### Problem and Requirements
Currently, a compactor is able to compact up to 20M timeseries within 2 hours for a level-2 compaction, including the time to download blocks, compact, and upload the newly compacted block. We would like to increase the timeseries limit per tenant, and compaction is one of the limiting factors. In addition, we would like to achieve the following:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot find a definition of "level-2" here or in Compactor docs either.

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pracucci
Copy link
Contributor

pracucci commented Jul 1, 2021

I'm going to merge the proposal to move forward but I would be glad if you could address Bryan comments in a follow up PR. Thanks!

@pracucci pracucci merged commit 297fb62 into cortexproject:master Jul 1, 2021
@ac1214 ac1214 mentioned this pull request Jul 10, 2021
3 tasks
@bboreham
Copy link
Contributor

@roystchiang gentle reminder I made some points about explaining terms, and I couldn't see any follow-up.

@roystchiang
Copy link
Contributor Author

@roystchiang gentle reminder I made some points about explaining terms, and I couldn't see any follow-up.

I will submit a follow up PR this week. Thanks for the reminder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants