Add -compactor.first-level-compaction-wait-period support #4401
Conversation
| @@ -673,15 +675,15 @@ func (c *MultitenantCompactor) compactUserWithRetries(ctx context.Context, userI | |||
| } | |||
|
|
|||
| func (c *MultitenantCompactor) compactUser(ctx context.Context, userID string) error { | |||
| bucket := bucket.NewUserBucketClient(userID, c.bucketClient, c.cfgProvider) | |||
| userBucket := bucket.NewUserBucketClient(userID, c.bucketClient, c.cfgProvider) | |||
There was a problem hiding this comment.
Note to reviewers: renamed just to clarify it's the "user bucket".
| reg := prometheus.NewRegistry() | ||
| defer c.syncerMetrics.gatherThanosSyncerMetrics(reg) | ||
|
|
||
| ulogger := util_log.WithUserID(userID, c.logger) | ||
| userLogger := util_log.WithUserID(userID, c.logger) |
There was a problem hiding this comment.
Note to reviewers: renamed just to clarify it's the "user logger".
pkg/compactor/job_wait_period.go
Outdated
| return true, nil | ||
| } | ||
|
|
||
| if ok, err := jobContainsBlocksUploadedAfter(ctx, job, userBucket, time.Now().Add(-waitPeriod)); err != nil { |
There was a problem hiding this comment.
nit: Maybe it makes sense to pass time.Now() as an argument to avoid calling it in a loop?
There was a problem hiding this comment.
I honestly don't think will make any visible difference. We're not calling it millions of times.
pkg/compactor/compactor_test.go
Outdated
| // We expect only 1 compaction job has been expected, while the 2nd has been skipped. | ||
| tsdbPlanner.AssertNumberOfCalls(t, "Plan", 1) | ||
|
|
||
| assert.ElementsMatch(t, []string{ |
There was a problem hiding this comment.
I know this isn't the first test to do this, but I really don't like asserting on side effects of an operation (info log statements) instead of the results (some sort of "compaction result" object?).
There was a problem hiding this comment.
I see your point. I've simplified it in 6e8f723. I'm still checking logs, but asserting only on 1 log line, which is what we care about in this test.
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci
force-pushed
the
add-compactor-grace-period
branch
from
fb7a88c
to
04a98e8
Compare
pkg/compactor/compactor_test.go
Outdated
|
|
||
| // Mock two tenants, each with the same exact overlapping blocks. However, | ||
| // the 2nd tenant has uploaded the blocks less than "wait period" ago. | ||
| bucketClient := &bucket.ClientMock{} |
There was a problem hiding this comment.
[nit] Could we use a real bucket instead? (inmem or filesystem) These "mocks" create brittle tests that are tightly coupled with the implementation (eg. we need to provide response for deletion-mark.json or no-compact-mark.json files, even if they are not important for this test at all).
There was a problem hiding this comment.
Good point. Done in 6e8f723.
pkg/compactor/compactor_test.go
Outdated
| // We expect only 1 compaction job has been expected, while the 2nd has been skipped. | ||
| tsdbPlanner.AssertNumberOfCalls(t, "Plan", 1) | ||
|
|
||
| assert.ElementsMatch(t, []string{ |
There was a problem hiding this comment.
Instead of matching all elements, we should only check for message that's relevant for this test, to avoid creating very fragile test that needs changes on every log message change.
Signed-off-by: Marco Pracucci <marco@pracucci.com>
…tLevelCompactionBlocksAndWaitPeriodNotElapsed to use a real bucket client and only assert on a specific log message Signed-off-by: Marco Pracucci <marco@pracucci.com>
What this PR does
In this PR I proposed to introduce
compactor.first-level-compaction-wait-periodconfiguration option to address the issue described by #4365.Notes:
Which issue(s) this PR fixes or relates to
Part of #4365
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]