add context cancellation checks on merging GetLabel slices #5837
Conversation
Signed-off-by: Erlan Zholdubai uulu <erlanz@amazon.com>
erlan-z
force-pushed
the
context-cancellation-checks-for-get-label
branch
from
23a7a65
to
b4bc204
Compare
| @@ -37,17 +38,18 @@ func StringsClone(s string) string { | |||
|
|
|||
| // MergeSlicesParallel merge sorted slices in parallel | |||
| // using the MergeSortedSlices function | |||
| func MergeSlicesParallel(parallelism int, a ...[]string) []string { | |||
| func MergeSlicesParallel(ctx context.Context, parallelism int, a ...[]string) ([]string, error) { | |||
There was a problem hiding this comment.
Can you add a test case that returns an error due to context check?
There was a problem hiding this comment.
I was not sure how to add proper test for context cancellation case.
- I could mock and add some wait time on iteration, and cancel context within this time. But adding wait time on tests? I'm not sure.
- Cancel context before calling the function? but in this one we are not actually testing the real case.
There should be better way than the above two. I will add second one, but let me know if we have any other option to properly test it.
There was a problem hiding this comment.
Cancel context before calling the function? but in this one we are not actually testing the real case.
I was thinking about in the middle of the call. If it is not easy to mock that, It is fine to skip.
There was a problem hiding this comment.
https://github.com/cortexproject/cortex/blob/master/pkg/querier/querier.go#L494
Do we want to add context check when we merge all responses?
Yes, we can add context check there as well, but the code that's used for merge is from thanos. I will create a PR in thanos for that but I think we can merge this PR separately |
erlan-z
force-pushed
the
context-cancellation-checks-for-get-label
branch
3 times, most recently
from
3724519
to
4210f3d
Compare
…angelog Signed-off-by: Erlan Zholdubai uulu <erlanz@amazon.com>
erlan-z
force-pushed
the
context-cancellation-checks-for-get-label
branch
from
4210f3d
to
d4a6a14
Compare
|
@alanprot Can you also take a look at this PR? Thanks |
|
I think looks good! |
What this PR does:
Add context error check while merging slices for GetLabel operations
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]