Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Ingester Search honor ctx errors #3031

Merged
merged 8 commits into from
Oct 18, 2023
Merged

Conversation

mdisibio
Copy link
Contributor

@mdisibio mdisibio commented Oct 17, 2023

What this PR does:
Big queries sent to Ingester.SearchRecent may keep running after a context timeout. The root cause is the Local backend wraps local file i/o which is not context aware. This fixes the Local backend to check errors, but also addresses some handling in instance.Search to properly wait for all goroutines to complete before releasing the lock.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@mdisibio mdisibio marked this pull request as ready for review October 18, 2023 11:36
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

looking good! do we need similar protections in our search tags/values endpoints? or in the metrics generator endpoints that take traceql?

modules/ingester/instance_search.go Outdated Show resolved Hide resolved
@@ -104,13 +116,21 @@ func (rw *Backend) CloseAppend(_ context.Context, tracker backend.AppendTracker)
return dst.Close()
}

func (rw *Backend) Delete(_ context.Context, name string, keypath backend.KeyPath, _ bool) error {
func (rw *Backend) Delete(ctx context.Context, name string, keypath backend.KeyPath, _ bool) error {
if err := ctx.Err(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

cc @zalegrala heads up you will want this context logic in the Blocks() method you added to the local backend

@mdisibio
Copy link
Contributor Author

do we need similar protections in our search tags/values endpoints? or in the metrics generator endpoints that take traceql?

The local backend and wal block fixes should address them by catching the context error on the next i/o. But would appreciate another set of eyes.

@joe-elliott
Copy link
Member

The local backend and wal block fixes should address them by catching the context error on the next i/o. But would appreciate another set of eyes.

Good call. Agreed!

@mdisibio mdisibio merged commit cc2820a into grafana:main Oct 18, 2023
13 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants