-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(bloom): Compute chunkrefs for series right before sending task to builder #14808
perf(bloom): Compute chunkrefs for series right before sending task to builder #14808
Conversation
… before sending to builder
1174064
to
9d6870f
Compare
pkg/bloombuild/planner/task.go
Outdated
*strategies.Task | ||
|
||
// We use forSeries in ToProtoTask to get the chunks for the series in the gaps. | ||
forSeries common.ClosableForSeries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between common.ClosableForSeries
and sharding.ForSeries
(which is used in strategies.Task
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common.ClosableForSeries
is
type ClosableForSeries interface {
sharding.ForSeries
Close() error
}
Added ForSeries
alias to the common pkg and used it instead of common.ClosableForSeries
and sharding.ForSeries
type ForSeries = sharding.ForSeries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just have a question about the improvement here
@@ -87,11 +89,23 @@ func GenBlock(ref bloomshipper.BlockRef) (bloomshipper.Block, error) { | |||
}, nil | |||
} | |||
|
|||
func GenSeries(bounds v1.FingerprintBounds) []*v1.Series { | |||
func GenSeries(bounds v1.FingerprintBounds) []model.Fingerprint { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Should this be GenFingerprint/GenFignerPrintWithStep?
|
||
// ToProtoTask converts a Task to a ProtoTask. | ||
// It will use the opened TSDB to get the chunks for the series in the gaps. | ||
func (t *Task) ToProtoTask(ctx context.Context, forSeries common.ForSeries) (*protos.ProtoTask, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
silly question: doesn't this move the memory overhead from NewTSDBSeriesIter to here? why does this use less memory?
if _, ok := alreadyOpen[idx]; ok { | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch :)
What this PR does / why we need it:
We've seen high memory usage in the planners that caused OOMs. We pulled a profile and saw most of the in-use memory was allocated by:
This PR improves this by not storing the chunkrefs in the tasks we store in the queue. Instead, we populate the chunkrefs just before sending the task to the builder.
Special notes for your reviewer:
We considered adding a memory limit to the queue and lazy loading the tasks but by doing so, we'd lose the metrics on the total number of tasks computed for a tenant, thus loosing the ability to track the build progress (% of completed tasks out of the total for a tenant).
We now have the following task definitions:
protos.ProtoTask
: The one we send to the builder through the wire. This one contains the chunkrefs.protos.Task
: Same asprotos.ProtoTask
but with Loki friendly types. Has aToProtoTask()
that converts it toprotos.ProtoTask
.strategies.Task
: inherits fromprotos.Task
. Overrides the Gaps fields to have series w/o chunks (only FPs).ToProtoTask()
takes forSeries as argument and populates the chunkrefs.planner.QueueTask
: Inherits fromstrategies.Task
and adds queue tracking info and forSeries to later on populate the chunks when transforming the queue task toprotos.ProtoTask
.ToProtoTask()
calls theToProtoTask()
fromstrategies.Task
passing theforSeries
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
deprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR