Skip to content

Query Frontend: Handle context error before decoding and merging responses #5499

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

Merged
merged 8 commits into from
Aug 5, 2023

Conversation

justinjung04
Copy link
Contributor

@justinjung04 justinjung04 commented Aug 4, 2023

What this PR does:

Decoding and merging response in query frontend can take a long time, and if the context has been cancelled in the process it should stop as well.

This PR adds context error check before heavy operations within decode and merge responses.

Which issue(s) this PR fixes:
n/a

Checklist

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

Signed-off-by: Justin Jung <[email protected]>
@justinjung04 justinjung04 changed the title Add context cancel check in query frontend Query Frontend: Handle context error before decoding and merging responses Aug 4, 2023
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks!

@yeya24
Copy link
Contributor

yeya24 commented Aug 4, 2023

golangci-lint run
pkg/querier/tripperware/instantquery/instant_query_test.go:385:4: lostcancel: the cancelCtx function is not used on all paths (possible context leak) (govet)
			ctx, cancelCtx := context.WithCancel(context.Background())
			^
pkg/querier/tripperware/instantquery/instant_query_test.go:401:6: lostcancel: this return statement may be reached without using the cancelCtx var defined on line 385 (govet)
					return
					^
pkg/querier/tripperware/queryrange/query_range_test.go:114:4: lostcancel: the cancelCtx function is not used on all paths (possible context leak) (govet)
			ctx, cancelCtx := context.WithCancel(context.Background())
			^
pkg/querier/tripperware/queryrange/query_range_test.go:126:5: lostcancel: this return statement may be reached without using the cancelCtx var defined on line 114 (govet)
				return
				^
pkg/querier/tripperware/queryrange/query_range_test.go:[7](https://github.com/cortexproject/cortex/actions/runs/5765792485/job/15632449186?pr=5499#step:6:8)12:4: lostcancel: the cancelCtx function is not used on all paths (possible context leak) (govet)
			ctx, cancelCtx := context.WithCancel(context.Background())
			^
pkg/querier/tripperware/queryrange/query_range_test.go:71[9](https://github.com/cortexproject/cortex/actions/runs/5765792485/job/15632449186?pr=5499#step:6:10):5: lostcancel: this return statement may be reached without using the cancelCtx var defined on line 7[12](https://github.com/cortexproject/cortex/actions/runs/5765792485/job/15632449186?pr=5499#step:6:13) (govet)
				return
				^

Need to fix this by calling cancelCtx in all test cases

Signed-off-by: Justin Jung <[email protected]>
Signed-off-by: Justin Jung <[email protected]>
@yeya24 yeya24 merged commit 6c41a64 into cortexproject:master Aug 5, 2023
@justinjung04 justinjung04 deleted the qfe-context-cancel branch March 26, 2025 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants