Repository navigation
Conversation
A partitioning compaction builds one series set per source block and merges them all in a single flat merge. Advancing that merge reads the winning block's series entry off disk, and every one of those reads is issued from the one goroutine that advances the merge heap, so the disk sees a queue depth near one however much IOPS the volume has provisioned. A compaction over a large number of blocks is then bound by read latency rather than by disk capacity, and provisioning more IOPS does not help it. Add -compactor.block-merge-fan-in to group the per-block series sets into contiguous subtrees before the final merge. Each subtree is wrapped in a background chunk series set, so it advances in its own goroutine and the reads behind the subtrees overlap. The flag is disabled by default (0), which keeps the single flat merge unchanged. BenchmarkMergeFanIn measures the effect. 64 sets, 1024 series replicated 3 ways, so 3136 set advances per op, with latency injected per advance: latency fan-in merge goroutines ns/op ns/advance vs flat 0 0 (off) 1 7519760 2398 -- 0 4 16 8696905 2773 0.86x 0 8 8 9024411 2878 0.83x 0 16 4 8390016 2675 0.90x 1ms 0 (off) 1 3451540907 1100619 -- 1ms 16 4 914257427 291536 3.8x 1ms 8 8 496123534 158203 6.9x 1ms 4 16 287013931 91522 12.0x The ns/advance column is the mechanism: a flat merge pays the full 1.10 ms per advance because every read queues behind one goroutine, and 16 goroutines bring that to 91.5 us without changing the read volume at all. Speedup tracks roughly 0.75x the goroutine count; the shortfall is the root merge's own serial handoff. Note the fan-in is inverted against goroutine count, since a smaller fan-in produces more subtrees. With no read latency to hide, the change costs 10-17%, which is why it is off by default. A workload whose blocks fit in page cache should leave it off. time.Sleep cannot resolve latencies below roughly 100us, so the benchmark reports ns/advance to show the latency a row actually achieved rather than the latency it asked for. TestMergeSubtreesMatchesFlatMerge asserts the grouped merge yields identical labels and samples to the flat merge across several fan-ins, including one that leaves a short final group. It compares samples flattened across chunk boundaries on purpose: a merge tree may lay the same data out in different chunks, which changes the output block's chunk count but not its data. Signed-off-by: Alex Le <leqiyue@amazon.com>
Signed-off-by: Alex Le <leqiyue@amazon.com>
alexqyle
force-pushed
the
compactor-block-merge-fan-in
branch
from
November 7, 2026 00:13
77fb910 to
1e8b5d7
Compare
yeya24
reviewed
Nov 7, 2026
| # Number of per-block series sets merged per subtree during compaction. Each | ||
| # subtree advances in its own goroutine, so a group of blocks larger than this | ||
| # value no longer issues every index read from a single goroutine. Only | ||
| # affects partitioning compaction. 0 disables it and keeps one flat merge over |
Contributor
There was a problem hiding this comment.
Is it possible for this to work for even non partitioning compaction?
Contributor
Author
There was a problem hiding this comment.
We can. The original code is mainly copied from Prometheus. We can either change Prometheus or create new populator in cortex for non partitioning
yeya24
approved these changes
Nov 7, 2026
golangci-lint's modernize linter rejects the three-clause loops, and the package's existing tests already use the range form. Signed-off-by: Alex Le <leqiyue@amazon.com>
make lint runs faillint with sync/atomic blocklisted in favour of go.uber.org/atomic, which this package already uses elsewhere. The Int64 Add and Load API is the same and its zero value is usable. Signed-off-by: Alex Le <leqiyue@amazon.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
A partitioning compaction builds one series set per source block and merges them all in a single flat merge. Advancing that merge reads the winning block's series entry off disk, and every one of those reads is issued from the one goroutine that advances the merge heap. The disk therefore sees a queue depth near one however much IOPS the volume has provisioned, and a compaction over a large number of blocks ends up bound by read latency rather than by disk capacity. Provisioning more IOPS does not help it.
This adds
-compactor.block-merge-fan-in, which groups the per-block series sets into contiguous subtrees before the final merge. Each subtree is wrapped in abackgrounChunkSeriesSet, so it advances in its own goroutine and the reads behind the subtrees overlap.Disabled by default (
0), which keeps the existing single flat merge unchanged.Benchmark
BenchmarkMergeFanIninjects latency per set advance to model the index read that backs a merge advance. 64 sets, 1024 series replicated 3 ways, so 3136 advances per op.goos: linux, goarch: amd64, cpu: AMD EPYC 7R13, 8 cores.The
ns/advancecolumn is the mechanism. A flat merge pays the full 1.10 ms per advance because every read queues behind one goroutine; 16 goroutines bring that to 91.5 us without changing the read volume at all. Speedup tracks roughly 0.75x the goroutine count, and the shortfall is the root merge's own serial handoff.Note the fan-in is inverted against goroutine count: a smaller fan-in produces more subtrees, so
fan_in=4over 64 sets gives 16 goroutines.With no read latency to hide, the change costs 10-17%, which is why it defaults to off. A workload whose blocks fit in page cache should leave it off. The flag is worth setting when a compaction merges many blocks off a volume with real read latency.
Two measurement notes baked into the benchmark, since both bit me while writing it:
time.Sleepcannot resolve latencies below roughly 100 us, and pinning to one CPU inflates every sleep. Either makes all rows collapse onto the timer floor and look identical. The benchmark reportsns/advanceso a row whose sleeps never resolved to the requested latency is visible rather than silently misleading.merge_goroutinesreports 1 for the flat case. A flat merge has 64 inputs but one goroutine advancing them, and the concurrency is what matters.Correctness
TestMergeSubtreesMatchesFlatMergeasserts the grouped merge yields identical labels and samples to the flat merge, across fan-ins 2, 3, 4 and 8 over 9 sets, so both the evenly-divided case and the short trailing group are covered.It compares samples flattened across chunk boundaries on purpose: a merge tree may lay the same data out in different chunks than a flat merge, which changes the output block's chunk count but not its data. The fixture gives a given timestamp the same value in every replica, which is how replicas present a series; with conflicting values at one timestamp, flat and tree order would legitimately differ and asserting equality would be testing undefined behaviour.
TestMergeSubtreesDisabledpins that fan-in 0, 1 and negative return the sets untouched.TestMergeSubtreesGroupingpins the subtree counts.Verified with a sabotage check: changing the grouping loop bound so it drops the trailing partial group fails exactly the expected subtests, while leaving
fan_in=3passing since 9 divides by 3.Memory stays comparable to the flat merge. The existing 1000-series lookahead is split across subtrees (
1000/subtrees, floor 8) rather than given to each one, so the buffered series count does not grow with the subtree count.Notes for review
CompactionLifecycleCallbackFactorygained ablockMergeFanIn intparameter. Existing tests needed no change, since they construct the callback with field names and assign the factory by type.NewBackgroundChunkSeriesSetkeeps its signature and delegates to a newNewBackgroundChunkSeriesSetWithBuffer, so current callers are unaffected.docs/andschemas/cortex-config-schema.jsonare regenerated output from the doc-generator, formake check-doc.#TODOfor the PR number; I'll follow up with the real number once this is open.golangci-lintwas not available in my environment, so lint coverage here isgo vetandgofmt -l. The full./pkg/compactor/suite passes.