Skip to content

Add --retries parameter to CLI commands #5748

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

Merged
merged 1 commit into from
Apr 18, 2025
Merged

Conversation

guilload
Copy link
Member

@guilload guilload commented Apr 15, 2025

Description

  • Add --retries parameter to CLI commands
  • Update OpenDAL

I got pulled into updating OpenDAL in order to use reqwest-middleware and reqwest-retry.

How was this PR tested?

  • Added unit test
  • Ran command locally with 10 retries against server that was not up yet. When I finally launched the server, the command succeeded

@guilload guilload requested a review from trinity-1686a April 15, 2025 18:05
@guilload guilload force-pushed the guilload/rest-client-retries branch from 5ee1e44 to eb9220b Compare April 15, 2025 19:06
.tls_built_in_root_certs(false)
.add_root_certificate(ca_cert);
}
let retry_policy = ExponentialBackoff::builder().build_with_max_retries(num_retries);
Copy link
Contributor

Choose a reason for hiding this comment

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

the default max backoff seems to be 30m, that seems overly long for a cli. i'd be in favor of capping that to a few minutes

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair, let me tweak the retry policy.


assert!(matches!(error, Error::Client(_)));
assert!(error.to_string().contains("tcp connect error"));
assert!(matches!(error, Error::Middleware(_)));
Copy link
Contributor

Choose a reason for hiding this comment

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

can we assert that we still say something about a tcp connect error? i don't know how this error stringify itself, but if it says nothing about the underlying situation, it's not really useful to the (human) consumer

@guilload guilload force-pushed the guilload/rest-client-retries branch 2 times, most recently from 0b14956 to c06c34d Compare April 18, 2025 16:10
@guilload guilload force-pushed the guilload/rest-client-retries branch from c06c34d to 96f34f5 Compare April 18, 2025 17:11
@guilload guilload merged commit de696d4 into main Apr 18, 2025
8 checks passed
@guilload guilload deleted the guilload/rest-client-retries branch April 18, 2025 18:07
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.

2 participants