[tempodb]: remove dead ring-based compaction code#6990
Draft
zalegrala wants to merge 4 commits intografana:mainfrom
Draft
[tempodb]: remove dead ring-based compaction code#6990zalegrala wants to merge 4 commits intografana:mainfrom
zalegrala wants to merge 4 commits intografana:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Removes the legacy ring-based compaction/retention entrypoints and loops from tempodb now that compaction/retention is driven by the backend scheduler/worker path (CompactWithConfig / RetainWithConfig). Updates tests to stop relying on the deleted setup path.
Changes:
- Deletes
EnableCompactionand the ring-loop based compaction/retention codepaths. - Removes
CompactorConfigfields that only applied to the deleted loops (max_time_per_tenant,compaction_cycle) and refactors tests to callCompactWithConfig/RetainWithConfigdirectly. - Simplifies/updates several tests that previously enabled compaction just to exercise polling/search behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tempodb/tempodb.go | Removes EnableCompaction from the Compactor interface and drops now-unused compactor fields from readerWriter. |
| tempodb/compactor.go | Deletes the ring-loop compaction orchestration helpers; leaves CompactWithConfig as the main entrypoint. |
| tempodb/retention.go | Removes the old retention loop wrapper, leaving RetainWithConfig / RetainTenantWithConfig. |
| tempodb/config.go | Removes defaults/fields related to the deleted loop path (max_time_per_tenant, compaction_cycle) and drops CompactorConfig.validate(). |
| tempodb/config_test.go | Removes the unit test that only validated the deleted CompactorConfig.validate() method. |
| tempodb/tempodb_test.go | Removes EnableCompaction usage in tests; updates one test to compact via CompactWithConfig. |
| tempodb/tempodb_search_test.go | Removes EnableCompaction setup from search test harnesses. |
| tempodb/tempodb_metrics_test.go | Removes EnableCompaction setup from metrics query test harness. |
| tempodb/retention_test.go | Refactors retention tests to call RetainWithConfig directly with explicit config. |
| tempodb/compactor_test.go | Removes tests/benchmarks that depended on the deleted ring-loop compaction code; keeps direct compaction tests. |
Comments suppressed due to low confidence (1)
tempodb/retention.go:18
- CRITICAL:
RetainWithConfigwill panic ifcompactorCfgis nil or ifRetentionConcurrencyis 0 (becauseboundedwaitgroup.New(0)panics). PreviouslyEnableCompactiondefaultedRetentionConcurrencyfor tests, but now callers can more easily hit this with misconfiguration (e.g.retention_concurrency: 0). Can we defensively default toDefaultRetentionConcurrency(or return early with a logged error) before constructing the bounded waitgroup?
func (rw *readerWriter) RetainWithConfig(ctx context.Context, compactorCfg *CompactorConfig, compactorSharder CompactorSharder, compactorOverrides CompactorOverrides) {
tenants := rw.blocklist.Tenants()
bg := boundedwaitgroup.New(compactorCfg.RetentionConcurrency)
The ring-based compaction loop was fully superseded by the backend scheduler/worker path. `EnableCompaction`, `compactionLoop`, `retentionLoop`, `doRetention`, `compactOneJob`, and related helpers were unreachable from any live code path. Remove them along with the `CompactorConfig` fields (`MaxTimePerTenant`, `CompactionCycle`) and `readerWriter` struct fields (`compactorCfg`, `compactorSharder`, `compactorOverrides`, `compactorTenantOffset`) that only existed to support that path. Retention tests are refactored to call `RetainWithConfig` directly with explicit config parameters. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add CompactorConfig.Validate() checking MaxCompactionRange > 0; wire into backendworker.ValidateConfig() so misconfiguration is caught at startup rather than causing a divide-by-zero panic in timeWindowBlockSelector.windowForTime() - Guard RetentionConcurrency == 0 in RetainWithConfig to avoid boundedwaitgroup.New(0) panic when field is explicitly zeroed in YAML - Remove max_time_per_tenant / compaction_cycle from the generated manifest and add a CHANGELOG entry for the removed fields Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
a644a1f to
fe23355
Compare
- Fix CHANGELOG ordering: [CHANGE] entry now precedes [BUGFIX] per convention documented in the PR template - Add TestCompactorConfigValidate in tempodb/config_test.go to pin the Validate() behavior (zero and non-zero compaction window) - Add TestValidateConfig in modules/backendworker/config_test.go to cover the new compactor validation path wired into ValidateConfig, including the zero-compaction-window failure case Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gisterFlagsAndApplyDefaults in backendworker test - Port TestCompactionRoundtrip from the ring-based path (compactOneJob) to CompactWithConfig. Verifies that after compacting N blocks: block count reduces, total object count is preserved, and every trace is findable with byte-identical content (proto.Equal). - Port BenchmarkCompaction to CompactWithConfig so compaction perf can still be tracked when new tempodb encoding versions are introduced. - Use RegisterFlagsAndApplyDefaults in backendworker TestValidateConfig so the base config picks up all defaults automatically rather than requiring manual field assignment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 PR does:
Removes the dead code left behind after the ring-based compaction path was superseded by the backend scheduler/worker architecture. Since compactors no longer run as separate ring members, the following were unreachable from any live code path and are deleted:
EnableCompaction(the entry point for the ring-based path)compactionLoop/compactOneTenant/compactWhileOwns/compactOneJob/doForAtLeastretentionLoop/doRetentionCompactorConfigfields:MaxTimePerTenant,CompactionCycle(including their defaults/flag registration)readerWriterstruct fields:compactorCfg,compactorSharder,compactorOverrides,compactorTenantOffsetCompactWithConfigandRetainWithConfig(called directly by the backend worker) are the only remaining compaction/retention paths and are kept.Retention tests are refactored to call
RetainWithConfigdirectly with explicit config parameters rather than relying on the now-deletedEnableCompactionsetup +doRetentionpattern.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]