-
Notifications
You must be signed in to change notification settings - Fork 1.8k
enhancement(aws_s3 sink): Add ability to configure request errors to retry #23206
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
enhancement(aws_s3 sink): Add ability to configure request errors to retry #23206
Conversation
…l errors. Added retry_all_errors to S3SinkConfig and S3RetryLogic structs. Setting retry_all_errors to the default value in the generate_config function. Added self.retry_all_errors to the condition in the is_retriable_error function. (vectordotdev#10870)
…. Changed retry_all_errors to Option<bool>.
… Added configured_to_retry and check_response functions to s3_common/config.rs. Added configured_to_retry call to is_retriable_error result in RetryLogic. (vectordotdev#10870)
…nd replaced them with retry_logic in S3SinkConfig. (vectordotdev#10870)
…ogic rather than unwrap_or. (vectordotdev#10870)
…umentation comment for errors_to_retry. (vectordotdev#10870)
…e/failed-response-retry
I'm trying to figure out the two failing checks -- Test Suite / Checks and Test Suite / Test Suite. In Test Suite / Checks, the failure is happening when Looking at .github/workflows/test.yml, maybe the Test Suite / Test Suite failure is simply because Test Suite / Checks failed. Appreciate any help. |
I will take a look @jchap-pnnl |
Great, thanks! |
Please take another look at the current state of this PR and let us know if this is ready for the final review. |
…Changed documentation comments explain that retry_strategy settings extend, not override, default retry behavior for the sink. (vectordotdev#10870)
I reran the tests, verified the functionality still works, and updated the PR description. I've addressed everything I'm aware of. Please resume reviewing. Thanks! |
Summary
Adds a new field, a RetryStrategy enum called retry_strategy, to S3SinkConfig that allows users to specify types of response errors to retry. Users can specify specific status codes of error responses of failed requests they want to be automatically retried, or they can specify all failed requests to be automatically retried.
Change Type
Is this a breaking change?
How did you test this PR?
access_key_id
to an invalid one. Verified the failed authentication service was retried when the configuration required it to and didn't when the configuration didn't require it to. Ran tests with different values forretry_strategy
,type
, andstatus_codes
:Used this configuration:
Does this PR include user facing changes?
Notes
@vectordotdev/vector
to reach out to us regarding this PR.pre-push
hook, please see this template.cargo fmt --all
cargo clippy --workspace --all-targets -- -D warnings
cargo nextest run --workspace
(alternatively, you can runcargo test --all
)./scripts/check_changelog_fragments.sh
git merge origin master
andgit push
.Cargo.lock
), pleaserun
cargo vdev build licenses
to regenerate the license inventory and commit the changes (if any). More details here.