Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: add retry to callApi #384

Merged
merged 1 commit into from
Apr 21, 2020
Merged

Conversation

etr2460
Copy link
Contributor

@etr2460 etr2460 commented Apr 21, 2020

🏆 Enhancements

Adds the fetch-retry package to automatically retry any requests that fail due to network issues or 503 errors. This seems like a pretty safe approach to apply to all requests, but would appreciate feedback, especially if you think it's likely to break something.

Test Plan:
add/update unit tests

to: @kristw @williaster @ktmud @rusackas

@etr2460 etr2460 requested a review from a team as a code owner April 21, 2020 00:03
@vercel
Copy link

vercel bot commented Apr 21, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/superset/superset-ui/f9jfvw1pg
✅ Preview: https://superset-ui-git-erik-ritter-connection-retries.superset.now.sh

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

wow this looks pretty sweet! left a couple comments but overall LGTM.

happy someone already wrote the fetch-retry package 😅

import { CallApi } from '../types';
import { CACHE_AVAILABLE, CACHE_KEY, HTTP_STATUS_NOT_MODIFIED, HTTP_STATUS_OK } from '../constants';

const RETRY_COUNT = 3;
const RETRY_DELAY_MS = 1000;
const RETRY_ON_HTTP_STATUS_CODES = [503];
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think we should make this configurable? like make this the default but override-able?

we could do that for all of the constants actually. no reason not to expose them for more granular customization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good call, I'll move it into the SupersetClient class.

@@ -437,7 +439,7 @@ describe('callApi()', () => {
});
});

it('rejects if the request throws', () => {
it('rejects after retrying thrice if the request throws', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

could update the tests to match the retry config variables specified if we expose them

Copy link
Contributor

Choose a reason for hiding this comment

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

this is pretty awesome tho that the throwing is delayed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

import { CallApi } from '../types';
import { CACHE_AVAILABLE, CACHE_KEY, HTTP_STATUS_NOT_MODIFIED, HTTP_STATUS_OK } from '../constants';

const RETRY_COUNT = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is a pretty big change, I wonder if we should make this 2 by default for a total of 3 requests? not really sure what's standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3 is the default for the package and is a standard i've seen before. if we make this configurable i think it's fine to default it though

@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #384 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #384      +/-   ##
==========================================
+ Coverage   26.35%   26.40%   +0.05%     
==========================================
  Files         192      192              
  Lines        5256     5260       +4     
  Branches      459      460       +1     
==========================================
+ Hits         1385     1389       +4     
  Misses       3842     3842              
  Partials       29       29              
Impacted Files Coverage Δ
.../superset-ui-connection/src/SupersetClientClass.ts 100.00% <100.00%> (ø)
...ages/superset-ui-connection/src/callApi/callApi.ts 100.00% <100.00%> (ø)
packages/superset-ui-connection/src/constants.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb9cce3...4193359. Read the comment docs.

@@ -16,6 +21,12 @@ export default function callApi({
stringify = true,
url,
Copy link
Contributor

@ktmud ktmud Apr 21, 2020

Choose a reason for hiding this comment

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

Can we add the retry options to callApi arguments, too, in case people want to override the defaults?

I'd also argue it's probably safer to make this an opt-in rather than opt-out. Can add an API to superset-ui/connection to change the defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the retry options to the SupersetClient config. However, I think it's fair to make this opt out. I can add a comment in incubator-superset that this was added, but I think the common path should include QOL features like automatic retried on network issues

@etr2460 etr2460 force-pushed the erik-ritter--connection-retries branch from dfb2ee8 to d5e6de2 Compare April 21, 2020 00:40
@vercel vercel bot temporarily deployed to Preview April 21, 2020 00:40 Inactive
@@ -40,6 +44,7 @@ export default class SupersetClientClass {
this.credentials = credentials;
this.csrfToken = csrfToken;
this.csrfPromise = undefined;
this.fetchRetryOptions = fetchRetryOptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this.fetchRetryOptions = { ...DEFAULT_FETCH_RETRY_OPTIONS, ...fetchRetryOptions } and fetchRetryOptions = {} in L33 in case people only want to override one of the options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call

@etr2460 etr2460 merged commit f4fdeb0 into master Apr 21, 2020
@delete-merged-branch delete-merged-branch bot deleted the erik-ritter--connection-retries branch April 21, 2020 01:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants