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

Implement Retry Mechanism for HttpExtractor #404

Merged
merged 7 commits into from
Jul 20, 2023
Merged

Conversation

georg-schwarz
Copy link
Member

Closes #403.

Example

block CarsExtractor oftype HttpExtractor {
		url: "https://gist.githubusercontent.com/noamross/e5dasdasd..."; // invalid URL
		retries: 3;
		retryBackoff: 10000;
	}

image

@georg-schwarz georg-schwarz requested a review from rhazn July 19, 2023 12:50
Copy link
Contributor

@rhazn rhazn left a comment

Choose a reason for hiding this comment

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

I think our default strategy should be exponential backoffs and we should enforce some form of minimal delay. Right now it is trivial to missconfigure (or deliberately configure in the case of JV cloud) this to spam a source with 1ms delayed requests.

I'd go with exponential backoff and minimal delay of 2s. So we'd have 2, 4, 8... etc backoffs.

libs/extensions/std/exec/src/http-extractor-executor.ts Outdated Show resolved Hide resolved
@georg-schwarz
Copy link
Member Author

georg-schwarz commented Jul 20, 2023

  • Added a prop retryBackoffStrategy with allowed values exponential (default) and linear.
  • Renamed retryBackoff to retryBackoffMilliseconds
  • Prop retryBackoffMilliseconds must be 1000 or larger; edit: 2000ms as minimum

@georg-schwarz georg-schwarz requested a review from rhazn July 20, 2023 10:10
Copy link
Contributor

@rhazn rhazn left a comment

Choose a reason for hiding this comment

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

It is wrong but intuitively, I'd assume exp backoff to be 1, 2, 4... for 1000ms. Maybe we force a min value of 2000 or do the 1,2,4 by hand?

I am fine with merging it like this as well though.

@georg-schwarz georg-schwarz merged commit 4d0ec1d into dev Jul 20, 2023
2 checks passed
@georg-schwarz georg-schwarz deleted the 403-http-retry branch July 20, 2023 12:07
@github-actions github-actions bot locked and limited conversation to collaborators Jul 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants