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

feat(deps): update tower crate to v0.5.2 and fix partial retry logic #22431

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Serendo
Copy link

@Serendo Serendo commented Feb 13, 2025

Summary

  1. Update the tower crate to 0.5 so that the retry function now takes a mutable reference of the request. which will be used to modify the original request when doing partial retries. As tower 0.5 has some breaking changes. Some args in the retry part are changed to mutable. Some service types are changed according to the order changed in the tower Buffer.
  2. Add a RetryPartial variant to RetryAction. This variant takes a closure which has the information of the response and will be used to modify the request so only sending the failed part of it.
  3. Implemented the RetryPartialFunction so that now retry_partial will only send those previously failed requests. those successful requests will not be sent again.

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes. This change will update tower to 0.5 which itself has breaking changes. but it should be transparent to users.
  • No

How did you test this PR?

I wrote a python server mocking the elasticsearch bulk api. which will give a response in which the first 5 documents are 201 created, and others are 429. I can see that a perticular document will only get one 201.

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

  • Please read our Vector contributor resources.
    • make check-all is a good command to run locally. This check is
      defined here. Some of these
      checks might not be relevant to your PR. For Rust changes, at the very least you should run:
      • cargo fmt --all
      • cargo clippy --workspace --all-targets -- -D warnings
      • cargo nextest run --workspace (alternatively, you can run cargo test --all)
  • If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
    run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

Closes: #140

#14891

- Adjustments of Buffer type definition in Tower 0.5
- As policy is directly modified in Tower 0.5. Removed the policy part in the RetryPolicyFuture.
- The change args to mutable for the retry function and clone_request function. It's changed in Tower 0.5.
- Changed the RetryPolicyFuture Output to (). This is also changed in Tower 0.5.
- Modified some tests as policy is now modified in advance function;
…ls a RetryPartialFunction trait to modify the old request.

In the elasticsearch sink. When there are errors in the response, the response's status_codes are keeped in a closure which is later used by the modify_request func in RetryPartialFunction trait.
@Serendo Serendo requested a review from a team as a code owner February 13, 2025 09:14
@bits-bot
Copy link

bits-bot commented Feb 13, 2025

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the domain: sinks Anything related to the Vector's sinks label Feb 13, 2025
@pront pront changed the title Es retry tower0.5 feat(deps): update tower crate to 0.5 and fix partial retry logic Feb 13, 2025
@pront pront changed the title feat(deps): update tower crate to 0.5 and fix partial retry logic feat(deps): update tower crate to v0.5.2 and fix partial retry logic Feb 13, 2025
@pront
Copy link
Member

pront commented Feb 13, 2025

Hi @Serendo, thank you for this PR! I think this is not a breaking change (from the perspective of Vector users). And also we will need a changelog to explain the fix.

@Serendo
Copy link
Author

Serendo commented Feb 14, 2025

Hi @Serendo, thank you for this PR! I think this is not a breaking change (from the perspective of Vector users). And also we will need a changelog to explain the fix.

I have added a changlog fragment for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: sinks Anything related to the Vector's sinks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elasticsearch partial bulk failures
3 participants