Skip to content
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

Call GetInstalledPackageResourceRefs with exp backoff. #4233

Merged
merged 2 commits into from
Feb 8, 2022

Conversation

absoludity
Copy link
Contributor

Signed-off-by: Michael Nelson minelson@vmware.com

Description of the change

See #4213 for description of the problem.

This change just ensures that the call will retry a number of times, backing off exponentially. For Flux it only needs to be a second, but carvel has other needs.

Benefits

Error no longer shown when first deploying flux package.

Possible drawbacks

Applicable issues

Additional information

Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Great! Thanks! +1 once CI passes

// with 500ms, 1000ms, 2000ms, 4000ms etc.
const wait = (ms: number) => new Promise(res => setTimeout(res, ms));

const callWithRetry = async (fn: any, initialWait: number, depth = 0): Promise<any> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we just use a library instead for also adding jitter to this exp backoff, like https://github.com/lifeomic/attempt ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@antgamdia thanks for the article about jitter, superinteresting!
That library looks easy to implement 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would we need jitter here? It's guaranteed to be a single client fetching the resource refs after creating the package... unless we coordinate a DDOS on a Kubeapps installation :P. (ie. we don't have the situation described in the article that leads to adding jitter to an exponential backoff, nor could we, unless I'm missing something?)

@antgamdia
Copy link
Contributor

By the way, do you think we should add some kind of spinner when waiting?

image

If so, (probably in another PR) we should let the ResourceTabs component know if the fetching attempts have been exhausted or not, I guess.

@absoludity absoludity force-pushed the 3695-check-installed-pkg-details-6 branch from e1a35df to 5b35ec8 Compare February 8, 2022 10:27
@absoludity absoludity force-pushed the 3695-check-installed-pkg-details-5 branch from d7437d4 to 660e74e Compare February 8, 2022 10:31
Base automatically changed from 3695-check-installed-pkg-details-5 to main February 8, 2022 11:19
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity absoludity force-pushed the 3695-check-installed-pkg-details-6 branch from 5b35ec8 to 8f07b43 Compare February 8, 2022 11:22
@absoludity absoludity merged commit e23269e into main Feb 8, 2022
@absoludity absoludity deleted the 3695-check-installed-pkg-details-6 branch February 8, 2022 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants