add context cancellation check at get series result iteration #13766
Conversation
Signed-off-by: Erlan Zholdubai uulu <erlanz@amazon.com>
web/api/v1/api.go
Outdated
| @@ -882,6 +882,9 @@ func (api *API) series(r *http.Request) (result apiFuncResult) { | |||
| warnings := set.Warnings() | |||
|
|
|||
| for set.Next() { | |||
| if err := ctx.Err(); err != nil { | |||
| return apiFuncResult{nil, contextErr(err), nil, nil} | |||
There was a problem hiding this comment.
Better to still return warnings and the closer?
return apiFuncResult{nil, contextErr(err), warnings, closer}
There was a problem hiding this comment.
thanks, added the warning and closer.
…and closer on error Signed-off-by: Erlan Zholdubai uulu <erlanz@amazon.com>
web/api/v1/api.go
Outdated
| @@ -896,6 +899,17 @@ func (api *API) series(r *http.Request) (result apiFuncResult) { | |||
| return apiFuncResult{metrics, nil, warnings, closer} | |||
| } | |||
|
|
|||
| func contextErr(err error) *apiError { | |||
There was a problem hiding this comment.
I see we already have a similar util: returnAPIError, maybe we can adjust and re-use it.
A regression test for this would be great.
There was a problem hiding this comment.
Thanks, I have changed to use existing returnAPIError. It covers context cancellation error, we can just use it.
For tests, I have added test for the case when context is cancelled while iteration. Other regression test cases looks to be covered by the existing tests.
web/api/v1/api_test.go
Outdated
| t.Run("should throw error when context cancelled on SeriesSet iteration", func(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
| timer := time.AfterFunc(10*time.Millisecond, func() { |
There was a problem hiding this comment.
I didn’t know the test would be so dependent on the implementation. I’m afraid that if the series body changes, we’ll have no guarantee that the context check you added is the one responsible for the cancellation.
I think we can go without a test for now. Perhaps we can just add a check for the error mapping in TestReturnAPIError. If you’d like, you can create an issue to discuss how we can improve the utils to better test such changes.
There was a problem hiding this comment.
Agree, this test doesn't look right with timer and at all. Removed test and added the mapping for context.Canceled error case.
Signed-off-by: Erlan Zholdubai uulu <erlanz@amazon.com>
erlan-z
force-pushed
the
check-context-cancellation-on-get-series
branch
from
184af8e
to
129321d
Compare
Signed-off-by: Erlan Zholdubai uulu <erlanz@amazon.com>
erlan-z
force-pushed
the
check-context-cancellation-on-get-series
branch
from
05ba797
to
29913e9
Compare
For series api when the response is big, we found out that it still will be iterating over the result SeriesSet even if query already timed out and context was cancelled.
This PR will add context check while iterating on the SeriesSet. There was similar context check changes here