alamb commented on code in PR #4915:
URL: https://github.com/apache/arrow-rs/pull/4915#discussion_r1353104089
##########
object_store/src/client/retry.rs:
##########
@@ -23,46 +23,50 @@ use futures::FutureExt;
use reqwest::header::LOCATION;
use reqwest::{Response, StatusCode};
use snafu::Error as SnafuError;
+use snafu::Snafu;
use std::time::{Duration, Instant};
use tracing::info;
/// Retry request error
-#[derive(Debug)]
-pub struct Error {
- retries: usize,
- message: String,
- source: Option<reqwest::Error>,
- status: Option<StatusCode>,
-}
-
-impl std::fmt::Display for Error {
- fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
- write!(
- f,
- "response error \"{}\", after {} retries",
- self.message, self.retries
- )?;
- if let Some(source) = &self.source {
- write!(f, ": {source}")?;
- }
- Ok(())
- }
-}
-
-impl std::error::Error for Error {
- fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
- self.source.as_ref().map(|e| e as _)
- }
+#[derive(Debug, Snafu)]
+pub enum Error {
+ #[snafu(display("Received redirect without LOCATION, this normally
indicates an incorrectly configured region"))]
+ BareRedirect,
+
+ #[snafu(display("Client error with status {status}: {}",
body.as_deref().unwrap_or("No Body")))]
+ Client {
+ status: StatusCode,
+ body: Option<String>,
+ },
+
+ #[snafu(display("Response error after {retries} retries: {source}"))]
Review Comment:
Isn't this kind of error normally called a `ClientError` or
`ConnectionError` as it denotes a problem communicating with the server?
##########
object_store/src/client/retry.rs:
##########
@@ -238,16 +242,14 @@ impl RetryExt for reqwest::RequestBuilder {
|| now.elapsed() > retry_timeout
|| !do_retry {
- return Err(Error{
+ return Err(Error::Response {
retries,
- message: "request error".to_string(),
- status: e.status(),
- source: Some(e),
+ source: e,
})
}
let sleep = backoff.next();
retries += 1;
- info!("Encountered request error ({}) backing off for
{} seconds, retry {} of {}", e, sleep.as_secs_f32(), retries, max_retries);
+ info!("Encountered transport error ({}) backing off
for {} seconds, retry {} of {}", e, sleep.as_secs_f32(), retries, max_retries);
Review Comment:
I think transport error makes sense ✅
##########
object_store/src/client/retry.rs:
##########
@@ -238,16 +242,14 @@ impl RetryExt for reqwest::RequestBuilder {
|| now.elapsed() > retry_timeout
|| !do_retry {
- return Err(Error{
+ return Err(Error::Response {
retries,
- message: "request error".to_string(),
- status: e.status(),
- source: Some(e),
+ source: e,
})
}
let sleep = backoff.next();
retries += 1;
- info!("Encountered request error ({}) backing off for
{} seconds, retry {} of {}", e, sleep.as_secs_f32(), retries, max_retries);
+ info!("Encountered transport error ({}) backing off
for {} seconds, retry {} of {}", e, sleep.as_secs_f32(), retries, max_retries);
Review Comment:
I think transport error makes sense ✅
##########
object_store/src/client/retry.rs:
##########
@@ -23,46 +23,50 @@ use futures::FutureExt;
use reqwest::header::LOCATION;
use reqwest::{Response, StatusCode};
use snafu::Error as SnafuError;
+use snafu::Snafu;
use std::time::{Duration, Instant};
use tracing::info;
/// Retry request error
-#[derive(Debug)]
-pub struct Error {
- retries: usize,
- message: String,
- source: Option<reqwest::Error>,
- status: Option<StatusCode>,
-}
-
-impl std::fmt::Display for Error {
- fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
- write!(
- f,
- "response error \"{}\", after {} retries",
- self.message, self.retries
- )?;
- if let Some(source) = &self.source {
- write!(f, ": {source}")?;
- }
- Ok(())
- }
-}
-
-impl std::error::Error for Error {
- fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
- self.source.as_ref().map(|e| e as _)
- }
+#[derive(Debug, Snafu)]
+pub enum Error {
+ #[snafu(display("Received redirect without LOCATION, this normally
indicates an incorrectly configured region"))]
+ BareRedirect,
+
+ #[snafu(display("Client error with status {status}: {}",
body.as_deref().unwrap_or("No Body")))]
Review Comment:
Isn't this techically a `ServerError` as is a value returned from the server
(rather than some error trying to make the connecton?)
##########
object_store/src/client/retry.rs:
##########
@@ -198,28 +199,31 @@ impl RetryExt for reqwest::RequestBuilder {
|| now.elapsed() > retry_timeout
|| !status.is_server_error() {
- // Get the response message if returned a
client error
- let message = match status.is_client_error() {
+ return Err(match status.is_client_error() {
true => match r.text().await {
- Ok(message) if !message.is_empty() =>
message,
- Ok(_) => "No Body".to_string(),
- Err(e) => format!("error getting
response body: {e}")
+ Ok(body) => {
+ Error::Client {
+ body: Some(body).filter(|b|
!b.is_empty()),
+ status,
+ }
+ }
+ Err(e) => {
+ Error::Response {
+ retries,
+ source: e,
+ }
+ }
}
- false => status.to_string(),
- };
-
- return Err(Error{
- message,
- retries,
- status: Some(status),
- source: Some(e),
- })
-
+ false => Error::Response {
+ retries,
+ source: e,
+ }
+ });
}
let sleep = backoff.next();
retries += 1;
- info!("Encountered server error, backing off for
{} seconds, retry {} of {}", sleep.as_secs_f32(), retries, max_retries);
+ info!("Encountered response error, backing off for
{} seconds, retry {} of {}", sleep.as_secs_f32(), retries, max_retries);
Review Comment:
I actually think `server error` is what I would expect. I would be confused
about what a `response error` was
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
