Skip to content

TSDB head compactor concurrency#2172

Merged
pracucci merged 21 commits intocortexproject:masterfrom
pstibrany:tsdb-head-compactor-concurrency
Feb 25, 2020
Merged

TSDB head compactor concurrency#2172
pracucci merged 21 commits intocortexproject:masterfrom
pstibrany:tsdb-head-compactor-concurrency

Conversation

@pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Feb 21, 2020 •
edited
Loading

What this PR does: This PR makes TSDB head compaction interval and concurrency configurable (defaults to 2h). Shipping now happens right after compaction, and previous options to control shipping interval and concurrency have been removed. Shipping can still be disabled.

Which issue(s) this PR fixes:
Fixes #2003

Checklist

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

@pstibrany pstibrany mentioned this pull request Feb 24, 2020
Closed
gouthamve
gouthamve reviewed Feb 24, 2020
View reviewed changes
Copy link
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, will 👍 after comments are addressed :)

gouthamve
gouthamve reviewed Feb 24, 2020
View reviewed changes
docs/operations/blocks-storage.md Outdated
Copy link
Contributor

@gouthamve gouthamve Feb 24, 2020

Choose a reason for hiding this comment

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

Hrm, this should be 5m and the tests should fail. Can you check why its not failing?

Copy link
Contributor Author

@pstibrany pstibrany Feb 24, 2020

Choose a reason for hiding this comment

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

I have edited this file manually. Somehow it isn't covered by Marco's tool, but haven't looked into more detail why yet. I'll fix the default.

Copy link
Contributor Author

@pstibrany pstibrany Feb 24, 2020

Choose a reason for hiding this comment

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

I think the reason is simply because block-storage is a separate file, while doc tool only generates config-file-reference.md. @pracucci surely knows better, I'm just guessing based on 5min looking around.

@pracucci
Copy link
Contributor

pracucci commented Feb 24, 2020

Fixes #2174

pracucci
pracucci reviewed Feb 24, 2020
View reviewed changes
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.

Good job @pstibrany and thanks for taking care of this! I left few comments, everything else LGTM 🎉

pkg/ingester/ingester_v2.go Outdated
Copy link
Contributor

@pracucci pracucci Feb 24, 2020

Choose a reason for hiding this comment

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

Why is it possible to disable compactions? We should never allow it. Even if you run without shipper you definitely wants compactions otherwise your memory will soon explode.

I would suggest:

  1. Add a config validation to ensure that HeadCompactionConcurrency > 0
  2. Always run compactionLoop()

What's your take?

Copy link
Contributor Author

@pstibrany pstibrany Feb 24, 2020

Choose a reason for hiding this comment

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

I left an option to disable it when running tests. But now I think it doesn't make sense even in that scenario. I'll modify it.

pkg/storage/tsdb/config.go Outdated
Copy link
Contributor

@pracucci pracucci Feb 24, 2020

Choose a reason for hiding this comment

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

5m looks a bit unfrequent to me. This is a very inexpensive operation and I would suggest to run it more frequently: 30s or 1m.

Copy link
Contributor Author

@pstibrany pstibrany Feb 24, 2020

Choose a reason for hiding this comment

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

Do we even need to make it configurable? I'd just hardcode 1m into the code.

Copy link
Contributor

@pracucci pracucci Feb 24, 2020

Choose a reason for hiding this comment

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

I thought about hardcoding to 1m as well, and that was my initial comment. I then changed my mind because I think being able to configure it could help future integration tests (ie. compact every 5s to have integration tests running faster). What's your take?

P.S. I'm talking about integration tests cause I would like to add at least 1 integration test testing the compaction and shipping soon.

Copy link
Contributor Author

@pstibrany pstibrany Feb 24, 2020

Choose a reason for hiding this comment

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

We need a secret option then ;-) I'd suggest adding a check to avoid setting it too high (> 5 mins).

Copy link
Contributor

@pracucci pracucci Feb 24, 2020

Choose a reason for hiding this comment

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

We need a secret option then ;-)

We can have a secret YAML config option with doc:"hidden" but I'm not sure it's possible to have an hidden CLI flag (I haven't investigated).

@pstibrany
Run local compactions on our own schedule and goroutines.
7b48087 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Fix and add some tests to validation.
e5bd9fc 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Refactor common code into runConcurrentUserWorkers method.
178f593 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Tied compaction and shipping together.
7e9e382 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Updated CHANGELOG.md
6440781 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Updated CHANGELOG.md
1454ae1 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Updated documentation.
d422afe 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Merge blocks.
4fe63a1 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Revert "Tied compaction and shipping together."
449f419 
This reverts commit ac372e9f065f00e909f67d7c3b3557e2974f311c.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Revert "Updated documentation."
0f60c01 
This reverts commit 9adb4cb9b4273ac189a3bb0478090966a0db67ec.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Fixed documentation.
6fee5b9 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Renamed TSDB head compaction options to use "head" prefix.
341a653 
To avoid confusion with compactor, which also has "compaction_interval"
config field.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Expose compaction metrics.
16ad22c 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Moved compaction metrics to TSDB state.
853346b 
They are now updated in Cortex code, when we call compaction.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Decrease compaction interval.
286265c 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Updated help and changelog entry.
4535ed3 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Documentation
87cbc73 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Fix default value.
8fee89b 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Review feedback:
cd72ae9 
- decrease default to 1 min
- compaction cannot be disabled
- compaction interval must be greater than 0, and <= 5 mins

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
pracucci
pracucci approved these changes Feb 24, 2020
View reviewed changes
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, with a nit. Thanks!

@pstibrany
Fix CHANGELOG.md
c0de26d 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany
Fix formatting to make lint happy.
10b65f9 
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pracucci pracucci merged commit 6d4da2c into cortexproject:master Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@pracucci pracucci pracucci approved these changes

@gouthamve gouthamve gouthamve approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Set a max concurrency TSDB blocks compaction

3 participants

@pstibrany @pracucci @gouthamve