Skip to content
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.

Sign up for GitHub

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

Perform ingester pushes in a background context rather than the request context #736

Merged
merged 2 commits into from
Mar 9, 2018
+9 −3
Merged

Perform ingester pushes in a background context rather than the request context #736

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c4d909
Perform ingester pushes in a background context rather than the reque…
csmarchbanks Mar 7, 2018
3823a14
Remove timeout on overall context, and add configured timeout to each…
csmarchbanks Mar 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion pkg/distributor/distributor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"golang.org/x/time/rate"

"github.com/go-kit/kit/log/level"
opentracing "github.com/opentracing/opentracing-go"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/common/model"
"github.com/prometheus/prometheus/pkg/labels"
Expand Down Expand Up @@ -350,7 +351,14 @@ func (d *Distributor) Push(ctx context.Context, req *client.WriteRequest) (*clie
defer cancel() // cancel the timeout to release resources
for ingester, samples := range samplesByIngester {
go func(ingester *ring.IngesterDesc, samples []*sampleTracker) {
d.sendSamples(ctx, ingester, samples, &pushTracker)
// Use a background context to make sure all ingesters get samples even if we return early
localCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need the WithTimeout on line 350?
Could we use the same timeout parameter?

Copy link
Contributor

Choose a reason for hiding this comment

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

It may well be the cancel() on line 351 that is causing the trouble, so it may not be necessary to start from Background() if you took that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like removing WithTimeout on line 350 and using the same timeout parameter for the background calls.

I believe if we don't have the background context, the context from our request is cancelled as soon as the response is returned.

defer cancel()
localCtx = user.InjectOrgID(localCtx, userID)
if sp := opentracing.SpanFromContext(ctx); sp != nil {
localCtx = opentracing.ContextWithSpan(localCtx, sp)
}
d.sendSamples(localCtx, ingester, samples, &pushTracker)
}(ingester, samples)
}
select {
Expand Down