Skip to content

feat(object_store): publicly expose client error #6158

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

feat(object_store): publicly expose client error #6158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ab31d5
feat: publicly expose client error
kyle-mccarthy Jul 30, 2024
3a04753
docs(object_store): add docs to `retry::Error`
kyle-mccarthy Jul 30, 2024
e796cf1
fix(object_store): keep `retry::Error::error()` crate public
kyle-mccarthy Jul 30, 2024
ef4bfc7
fix(object_store): conditionally compile `retry::Error`
kyle-mccarthy Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion object_store/src/client/retry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,32 @@ use tracing::info;
/// Retry request error
#[derive(Debug, Snafu)]
pub enum Error {
/// The response sent by the cloud provider was a redirect without a
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do this I think we should make the variants opaque, and expose information with accessors. Otherwise it is going to be very hard to evolve this without introducing breaking changes

Copy link
Contributor

Choose a reason for hiding this comment

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

I think keeping the underlying error opaque is pretty hard to use and clearly there appears to be a need to get finer grained information about what the error was

Otherwise it is going to be very hard to evolve this without introducing breaking changes

Perhaps another possibility is to mark it as [#non_exhaustive] https://doc.rust-lang.org/reference/attributes/type_system.html

If we do this I think we should make the variants opaque, and expose information with accessors.

Are you suggesting something like

impl Error {
  fn is_redirect(&self) -> bool {..}
  fn status_code(&self) -> Option<StatusCode> {..}
...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you suggesting something like

Yes, this is also what the underlying HTTP clients do as well. My aprehension is we're exposing a fair amount of internal details around how we perform requests, retries, etc... that seems undesirable.

non-exhaustive would help for adding new variants, but we'd be unable to change existing. If say we wanted to add more context to a retry error message, this would be a breaking change

/// location header. This normally indicates an incorrectly configured region.
#[snafu(display("Received redirect without LOCATION, this normally indicates an incorrectly configured region"))]
BareRedirect,

/// The client received an error response that cannot be retried.
#[snafu(display("Client error with status {status}: {}", body.as_deref().unwrap_or("No Body")))]
Client {
/// The HTTP status code.
status: StatusCode,
/// The error body.
body: Option<String>,
},

/// The number of retries was exhausted or the time limit was reached.
#[snafu(display("Error after {retries} retries in {elapsed:?}, max_retries:{max_retries}, retry_timeout:{retry_timeout:?}, source:{source}"))]
Reqwest {
/// The number of retries attempted.
retries: usize,
/// The maximum number of retries allowed.
max_retries: usize,
/// The time elapsed since the first attempt.
elapsed: Duration,
/// The maximum amount of time to spend retrying.
retry_timeout: Duration,
/// The error that occurred while processing a Request.
source: reqwest::Error,
},
}
Expand All @@ -68,7 +79,8 @@ impl Error {
}
}

pub fn error(self, store: &'static str, path: String) -> crate::Error {
#[cfg(any(feature = "aws", feature = "gcp", feature = "azure"))]
pub(crate) fn error(self, store: &'static str, path: String) -> crate::Error {
match self.status() {
Some(StatusCode::NOT_FOUND) => crate::Error::NotFound {
path,
Expand Down
4 changes: 2 additions & 2 deletions object_store/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,8 +526,8 @@ mod client;

#[cfg(feature = "cloud")]
pub use client::{
backoff::BackoffConfig, retry::RetryConfig, ClientConfigKey, ClientOptions, CredentialProvider,
StaticCredentialProvider,
backoff::BackoffConfig, retry::Error as ClientError, retry::RetryConfig, ClientConfigKey,
ClientOptions, CredentialProvider, StaticCredentialProvider,
};

#[cfg(feature = "cloud")]
Expand Down
Loading