feat: allow overriding retry validator at request level #876

cemreyavuz · 2022-09-14T13:05:34Z

In brief, the motivation is allowing to override retry behavior at the request level. Slack thread can be checked for more background.

In the PR, I extended the http request options with a field called retryValidator. It can be either a function or a boolean with the value false. If it is false, we don't retry at all. If it is a function, we call it to decide whether retry or not. Else, we use the default retry validator.

There are a few ways we can go for the solution. I went with the generic solution to include extra options for requests inside the BasicHttpClient. However, I wouldn't say I'm too happy with it. We need to pass the extra props for retry validator basically everywhere.

Instead of extending BasicCogniteClient to add retry functionality, we can implement this inside that class with an option to disable globally (which is actually the idea with RetryableHttpClient in the end as well). This might be one alternative.

packages/core/src/httpClient/basicHttpClient.ts

cemreyavuz · 2022-09-20T08:27:41Z

Integration test probably fails because of the actual api

cemreyavuz · 2022-09-20T08:28:41Z

packages/core/src/httpClient/retryableHttpClient.ts

+      const retryValidator =
+        typeof request.retryValidator === 'function'
+          ? request.retryValidator
+          : this.retryValidator;
+
+      const shouldRetry =
+        request.retryValidator !== false &&
+        retryValidator(request, response, retryCount);


This is basically the whole change, rest is just type updates

polomani · 2022-09-20T12:44:14Z

@cemreyavuz what's the use case? what do you actually want to achieve?

cemreyavuz · 2022-09-20T14:03:31Z

Forgot to add the slack thread link: https://cognitedata.slack.com/archives/CG10VQPFX/p1663074669728439. In the most basic sense, I want to override the retry behavior for some requests. I want to stop retrying for some, I want to change the retry behavior for some other (like, retry 3 times instead of 5, or change the back-off mechanism).

BugGambit · 2022-09-21T09:13:08Z

What about adding an optional retryValidator?: RetryValidator field here: https://github.com/cognitedata/cognite-sdk-js/blob/master/packages/core/src/baseCogniteClient.ts#L30-L50
And modify the getRetryValidator method here to check if the user passed a custom retryValidator?
Wouldn't that solve the problem without propagating changes down to BasicHttpClient?

cemreyavuz · 2022-09-21T10:09:59Z

What about adding an optional retryValidator?: RetryValidator field here: https://github.com/cognitedata/cognite-sdk-js/blob/master/packages/core/src/baseCogniteClient.ts#L30-L50
And modify the getRetryValidator method here to check if the user passed a custom retryValidator?
Wouldn't that solve the problem without propagating changes down to BasicHttpClient?

I think we should also add that option "in addition" to this one - that was actually going to be my next PR.

I agree that it is a bit complex to propagate these changes down to BasicHttpClient - but I think we should still give the option to stop retrying per request. We don't have to override the function itself, so it can be a boolean. But even that wouldn't simplify this code necessarily.

You might ask why overriding the global retry validator is not enough. The fusion specific reason is that there are specific needs for each subapp for retries. The main motivation for doing this at request level is not retrying rather "unimportant" requests to unblock other "important" requests. One example from transformations sub app: we make tons of requests to raw api for fetching the tables in databases for tooltips on sidebar. When the number of requests gets bigger, we start getting 429s for raw api requests. However, we actually make some other requests to raw api in the same page that is more critical for UI to function. So when we handle these at request level, either we can stop retrying lower priority requests - or we can higher the back-off period to let higher priority requests go through first.

Also another problem with having one retry validator on global level is that we can only manage that in sdk-singleton. Subapps wouldn't be able to change the behavior at all. So, subapp developers would go into sdk-singleton and update the validator as it fits their use cases. And, there is still prioritization problem specific to each subapp. So, overall I still think we should have an option to either stop or override retrying at request level.

BugGambit

I left a small comment.

I get the point that a global retry validator is not sufficient for every use case.
But I still believe we should not modify BasicHttpClient. That is just an abstraction for Axios/fetch etc., and shouldn't care about retries. If we do that, we mix the abstraction levels.
I suggest to modify RetryableHttpClient where we override the http-methods (get, post, put, delete, patch) from BasicHttpClient providing an additional option to have a custom retry validator.

What do you think?

packages/core/src/httpClient/basicHttpClient.ts

cemreyavuz · 2022-09-22T10:22:00Z

I left a small comment.

I get the point that a global retry validator is not sufficient for every use case.
But I still believe we should not modify BasicHttpClient. That is just an abstraction for Axios/fetch etc., and shouldn't care about retries. If we do that, we mix the abstraction levels.
I suggest to modify RetryableHttpClient where we override the http-methods (get, post, put, delete, patch) from BasicHttpClient providing an additional option to have a custom retry validator.

What do you think?

I agree on not modifying BasicHttpClient, that's why I went the way of extending type with ExtraOptions for requests . But doing all of these changes inside RetryableHttpClient also works - actually it's even better imo. We have more control over it this way and it's less error prone for the future.

~~(I'll also add a few tests for this new functionality - but wanted to get the review for the actual code in the meanwhile)~~

cemreyavuz · 2022-09-22T10:58:53Z

@BugGambit we can also add a test to check if default max retry attemps number is used - but the same problem applies here as well: #878 (comment). I'm open for suggestions if there is a way to make it work

BugGambit

Nice. I liked this approach better. I left a few comments.

packages/core/src/httpClient/retryableHttpClient.ts

BugGambit · 2022-09-22T14:23:47Z

packages/core/src/httpClient/retryableHttpClient.ts

+  HttpRequestRetryValidatorOptions;
+
+type HttpRequestRetryValidatorOptions = {
+  retryValidator?: false | RetryValidator;


So the behaviour is as follows:

null/undefined -> default retrier

false -> no retries at all

function -> custom retrier
?

Yes, the motivation was flexibility - but of course that comes with a small cost on cognitive complexity.

We can even go one step further and add the option of numbers for retry counts in the future as well - sth like this: https://tanstack.com/query/v4/docs/guides/query-retries. I didn't want to have it as I was forcing a max retry count - but if we won't have it, we can have that option as well. In that case the behaviour would be:

undefined-> default retry validator

false -> no retry

function -> custom retry validator

number -> just check retry count and retry if it doesn't exceed the argument

packages/core/src/httpClient/retryValidator.ts

packages/core/src/httpClient/retryableHttpClient.ts

cemreyavuz · 2022-09-23T08:37:08Z

~~@BugGambit should I also bump the version, or does ci/cd do it automatically?~~ (i think it does, so I'm not changing)

codecov · 2022-09-23T08:44:10Z

Codecov Report

Merging #876 (f520340) into master (de3557f) will decrease coverage by 4.54%.
The diff coverage is 69.24%.

@@            Coverage Diff             @@
##           master     #876      +/-   ##
==========================================
- Coverage   84.06%   79.52%   -4.55%     
==========================================
  Files          70      105      +35     
  Lines        1952     3389    +1437     
  Branches      249      510     +261     
==========================================
+ Hits         1641     2695    +1054     
- Misses        301      671     +370     
- Partials       10       23      +13

Impacted Files	Coverage Δ
packages/beta/src/cogniteClient.ts	`100.00% <ø> (ø)`
packages/core/src/httpClient/httpError.ts	`90.00% <ø> (ø)`
packages/core/src/testUtils.ts	`35.59% <0.00%> (-19.58%)`	⬇️
packages/stable/src/__tests__/testUtils.ts	`96.42% <ø> (-3.58%)`	⬇️
packages/stable/src/api/3d/assetMappings3DApi.ts	`79.16% <ø> (-20.84%)`	⬇️
packages/stable/src/api/3d/nodes3DApi.ts	`81.81% <ø> (-18.19%)`	⬇️
packages/stable/src/api/3d/revealNodes3DApi.ts	`100.00% <ø> (ø)`
packages/stable/src/api/3d/revealSectors3DApi.ts	`100.00% <ø> (ø)`
packages/stable/src/api/3d/revisions3DApi.ts	`95.65% <ø> (-4.35%)`	⬇️
...kages/stable/src/api/annotations/annotationsApi.ts	`100.00% <ø> (ø)`
... and 124 more

BugGambit

LGTM 🚀

BugGambit · 2022-09-23T10:13:47Z

~~@BugGambit should I also bump the version, or does ci/cd do it automatically?~~ (i think it does, so I'm not changing)

Correct. It will bump it automatically based on the commit messages.

cemreyavuz changed the title ~~chore: wip~~ chore: allow overriding retry validator at request level Sep 14, 2022

cemreyavuz force-pushed the feat/DX-907 branch from 9803cc0 to 58e39d3 Compare September 20, 2022 08:15

cemreyavuz commented Sep 20, 2022

View reviewed changes

packages/core/src/httpClient/basicHttpClient.ts Outdated Show resolved Hide resolved

cemreyavuz changed the title ~~chore: allow overriding retry validator at request level~~ feat: allow overriding retry validator at request level Sep 20, 2022

cemreyavuz marked this pull request as ready for review September 20, 2022 08:25

cemreyavuz requested a review from a team September 20, 2022 08:25

cemreyavuz requested review from BugGambit, andersfylling and polomani as code owners September 20, 2022 08:25

cemreyavuz commented Sep 20, 2022

View reviewed changes

BugGambit requested changes Sep 22, 2022

View reviewed changes

packages/core/src/httpClient/basicHttpClient.ts Outdated Show resolved Hide resolved

cemreyavuz force-pushed the feat/DX-907 branch from 58e39d3 to 8e3d019 Compare September 22, 2022 10:17

cemreyavuz requested a review from BugGambit September 22, 2022 10:22

cemreyavuz requested a review from vegardok September 22, 2022 11:04

cemreyavuz mentioned this pull request Sep 22, 2022

feat: update 401 handler logic #878

Merged

BugGambit reviewed Sep 22, 2022

View reviewed changes

cemreyavuz force-pushed the feat/DX-907 branch from 3cb3827 to ec135f7 Compare September 23, 2022 08:18

BugGambit reviewed Sep 23, 2022

View reviewed changes

packages/core/src/httpClient/retryValidator.ts Outdated Show resolved Hide resolved

packages/core/src/httpClient/retryableHttpClient.ts Outdated Show resolved Hide resolved

packages/core/src/httpClient/retryableHttpClient.ts Outdated Show resolved Hide resolved

cemreyavuz requested a review from BugGambit September 23, 2022 08:38

cemreyavuz force-pushed the feat/DX-907 branch from 0cce54c to 55217ba Compare September 23, 2022 10:12

BugGambit approved these changes Sep 23, 2022

View reviewed changes

cemreyavuz added 7 commits September 23, 2022 12:15

feat: allow overriding retry validator at request level

088e8b8

feat: respect default value for max retry attempts value

ce3fc56

test: check if retryValidator is respected

36af630

chore: rename DEFAULT_MAX_RETRY_ATTEMPTS

4f418d3

chore: name request argument

6917956

chore: use lodash/isFunction for function type check

eae2bab

test: update type for http request

f520340

cemreyavuz force-pushed the feat/DX-907 branch from 55217ba to f520340 Compare September 23, 2022 10:15

cemreyavuz merged commit 39807e9 into master Sep 23, 2022

cemreyavuz deleted the feat/DX-907 branch September 23, 2022 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow overriding retry validator at request level #876

feat: allow overriding retry validator at request level #876

cemreyavuz commented Sep 14, 2022 •

edited

Loading

cemreyavuz commented Sep 20, 2022

cemreyavuz Sep 20, 2022

polomani commented Sep 20, 2022

cemreyavuz commented Sep 20, 2022 •

edited

Loading

BugGambit commented Sep 21, 2022

cemreyavuz commented Sep 21, 2022 •

edited

Loading

BugGambit left a comment

cemreyavuz commented Sep 22, 2022 •

edited

Loading

cemreyavuz commented Sep 22, 2022

BugGambit left a comment

BugGambit Sep 22, 2022

cemreyavuz Sep 23, 2022

cemreyavuz commented Sep 23, 2022 •

edited

Loading

codecov bot commented Sep 23, 2022 •

edited

Loading

BugGambit left a comment

BugGambit commented Sep 23, 2022

feat: allow overriding retry validator at request level #876

feat: allow overriding retry validator at request level #876

Conversation

cemreyavuz commented Sep 14, 2022 • edited Loading

cemreyavuz commented Sep 20, 2022

cemreyavuz Sep 20, 2022

Choose a reason for hiding this comment

polomani commented Sep 20, 2022

cemreyavuz commented Sep 20, 2022 • edited Loading

BugGambit commented Sep 21, 2022

cemreyavuz commented Sep 21, 2022 • edited Loading

BugGambit left a comment

Choose a reason for hiding this comment

cemreyavuz commented Sep 22, 2022 • edited Loading

cemreyavuz commented Sep 22, 2022

BugGambit left a comment

Choose a reason for hiding this comment

BugGambit Sep 22, 2022

Choose a reason for hiding this comment

cemreyavuz Sep 23, 2022

Choose a reason for hiding this comment

cemreyavuz commented Sep 23, 2022 • edited Loading

codecov bot commented Sep 23, 2022 • edited Loading

Codecov Report

BugGambit left a comment

Choose a reason for hiding this comment

BugGambit commented Sep 23, 2022

cemreyavuz commented Sep 14, 2022 •

edited

Loading

cemreyavuz commented Sep 20, 2022 •

edited

Loading

cemreyavuz commented Sep 21, 2022 •

edited

Loading

cemreyavuz commented Sep 22, 2022 •

edited

Loading

cemreyavuz commented Sep 23, 2022 •

edited

Loading

codecov bot commented Sep 23, 2022 •

edited

Loading