Skip to content
Merged
54 changes: 30 additions & 24 deletions components/spider-client/src/grpc/job.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,11 @@ impl JobOrchestrationClient {
compressed_serialized_task_graph,
compressed_serialized_inputs,
};
let response = call_with_retry(self.retry_config, async || {
self.connection_pool
.get_client()
.register_job(request.clone())
.await
let pool = self.connection_pool.clone();
let response = call_with_retry(self.retry_config, move || {
let mut client = pool.get_client();
let request = request.clone();
async move { client.register_job(request).await }
})
.await
.map_err(|status| job_status_to_error(&status))?
Expand All @@ -119,8 +119,11 @@ impl JobOrchestrationClient {
let request = storage::JobIdRequest {
job_id: job_id.get(),
};
let response = call_with_retry(self.retry_config, async || {
self.connection_pool.get_client().start_job(request).await
let pool = self.connection_pool.clone();
let response = call_with_retry(self.retry_config, move || {
let mut client = pool.get_client();
let request = request;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
async move { client.start_job(request).await }
})
.await
.map_err(|status| job_status_to_error(&status))?
Expand All @@ -145,8 +148,11 @@ impl JobOrchestrationClient {
let request = storage::JobIdRequest {
job_id: job_id.get(),
};
let response = call_with_retry(self.retry_config, async || {
self.connection_pool.get_client().cancel_job(request).await
let pool = self.connection_pool.clone();
let response = call_with_retry(self.retry_config, move || {
let mut client = pool.get_client();
let request = request;
async move { client.cancel_job(request).await }
})
.await
.map_err(|status| job_status_to_error(&status))?
Expand All @@ -171,11 +177,11 @@ impl JobOrchestrationClient {
let request = storage::JobIdRequest {
job_id: job_id.get(),
};
let response = call_with_retry(self.retry_config, async || {
self.connection_pool
.get_client()
.get_job_state(request)
.await
let pool = self.connection_pool.clone();
let response = call_with_retry(self.retry_config, move || {
let mut client = pool.get_client();
let request = request;
async move { client.get_job_state(request).await }
})
.await
.map_err(|status| job_status_to_error(&status))?
Expand All @@ -202,11 +208,11 @@ impl JobOrchestrationClient {
let request = storage::JobIdRequest {
job_id: job_id.get(),
};
let response = call_with_retry(self.retry_config, async || {
self.connection_pool
.get_client()
.get_job_outputs(request)
.await
let pool = self.connection_pool.clone();
let response = call_with_retry(self.retry_config, move || {
let mut client = pool.get_client();
let request = request;
async move { client.get_job_outputs(request).await }
})
.await
.map_err(|status| job_status_to_error(&status))?
Expand All @@ -231,11 +237,11 @@ impl JobOrchestrationClient {
let request = storage::JobIdRequest {
job_id: job_id.get(),
};
let response = call_with_retry(self.retry_config, async || {
self.connection_pool
.get_client()
.get_job_error(request)
.await
let pool = self.connection_pool.clone();
let response = call_with_retry(self.retry_config, move || {
let mut client = pool.get_client();
let request = request;
async move { client.get_job_error(request).await }
})
.await
.map_err(|status| job_status_to_error(&status))?
Expand Down
20 changes: 10 additions & 10 deletions components/spider-client/src/grpc/resource_group.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,11 @@ impl ResourceGroupManagementClient {
external_resource_group_id,
password,
};
let response = call_with_retry(self.retry_config, async || {
self.connection_pool
.get_client()
.add_resource_group(request.clone())
.await
let pool = self.connection_pool.clone();
let response = call_with_retry(self.retry_config, move || {
let mut client = pool.get_client();
let request = request.clone();
async move { client.add_resource_group(request).await }
})
.await
.map_err(|status| resource_group_status_to_error(&status))?
Expand Down Expand Up @@ -107,11 +107,11 @@ impl ResourceGroupManagementClient {
resource_group_id: resource_group_id.get(),
password,
};
call_with_retry(self.retry_config, async || {
self.connection_pool
.get_client()
.verify_resource_group(request.clone())
.await
let pool = self.connection_pool.clone();
call_with_retry(self.retry_config, move || {
let mut client = pool.get_client();
let request = request.clone();
async move { client.verify_resource_group(request).await }
})
.await
.map_err(|status| resource_group_status_to_error(&status))?;
Expand Down
54 changes: 29 additions & 25 deletions components/spider-utils/src/grpc/retry.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! An async retry helper for transient gRPC call failures.

use std::error::Error;
use std::future::Future;
use std::time::Duration;

use rand::Rng;
Expand Down Expand Up @@ -43,7 +44,8 @@ impl Default for RetryConfig {
///
/// * `ResponseType` - The success value produced by `grpc_call`.
/// * `ErrorType` - The error produced by `grpc_call`.
/// * `GrpcCall` - The async closure performing the gRPC call.
/// * `GrpcCall` - The closure performing the gRPC call.
/// * `FutureType` - The `Send` future returned by `grpc_call`.
/// * `RetriableCheck` - Classifies an error as retriable or not.
///
/// # Returns
Expand All @@ -60,7 +62,8 @@ impl Default for RetryConfig {
pub async fn execute_with_retry<
ResponseType,
ErrorType,
GrpcCall: AsyncFnMut() -> Result<ResponseType, ErrorType>,
GrpcCall: FnMut() -> FutureType,
FutureType: Future<Output = Result<ResponseType, ErrorType>> + Send,
RetriableCheck: Fn(&ErrorType) -> bool,
>(
max_retries: usize,
Expand Down Expand Up @@ -88,7 +91,8 @@ pub async fn execute_with_retry<
/// # Type Parameters
///
/// * `ResponseType` - The success value produced by `grpc_call`.
/// * `GrpcCall` - The async closure performing the gRPC round-trip.
/// * `GrpcCall` - The closure performing the gRPC round-trip.
/// * `FutureType` - The `Send` future returned by `grpc_call`.
///
/// # Returns
///
Expand All @@ -102,7 +106,8 @@ pub async fn execute_with_retry<
/// budget is exhausted.
pub async fn call_with_retry<
ResponseType,
GrpcCall: AsyncFnMut() -> Result<ResponseType, Status>,
GrpcCall: FnMut() -> FutureType,
FutureType: Future<Output = Result<ResponseType, Status>> + Send,
>(
retry_config: RetryConfig,
grpc_call: GrpcCall,
Expand Down Expand Up @@ -164,7 +169,8 @@ fn backoff(retry: usize, max_backoff: Duration) -> Duration {

#[cfg(test)]
mod tests {
use std::cell::Cell;
use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering;
use std::time::Duration;

use tonic::Code;
Expand All @@ -180,76 +186,75 @@ mod tests {

#[tokio::test]
async fn succeeds_on_first_attempt() {
let calls = Cell::new(0usize);
let calls = AtomicUsize::new(0);
let result: Result<i32, i32> = execute_with_retry(
3,
TEST_MAX_BACKOFF,
async || {
calls.set(calls.get() + 1);
calls.fetch_add(1, Ordering::Relaxed);
Ok(42)
},
|_error| true,
)
.await;

assert_eq!(result, Ok(42));
assert_eq!(calls.get(), 1);
assert_eq!(calls.load(Ordering::Relaxed), 1);
}

#[tokio::test]
async fn succeeds_after_retriable_failures() {
let calls = Cell::new(0usize);
let calls = AtomicUsize::new(0);
let result: Result<i32, i32> = execute_with_retry(
5,
TEST_MAX_BACKOFF,
async || {
let attempt = calls.get();
calls.set(attempt + 1);
let attempt = calls.fetch_add(1, Ordering::Relaxed);
if attempt < 2 { Err(-1) } else { Ok(7) }
},
|_error| true,
)
.await;

assert_eq!(result, Ok(7));
assert_eq!(calls.get(), 3);
assert_eq!(calls.load(Ordering::Relaxed), 3);
}

#[tokio::test]
async fn non_retriable_error_returns_immediately() {
let calls = Cell::new(0usize);
let calls = AtomicUsize::new(0);
let result: Result<i32, i32> = execute_with_retry(
3,
TEST_MAX_BACKOFF,
async || {
calls.set(calls.get() + 1);
calls.fetch_add(1, Ordering::Relaxed);
Err(99)
},
|error| *error != 99,
)
.await;

assert_eq!(result, Err(99));
assert_eq!(calls.get(), 1);
assert_eq!(calls.load(Ordering::Relaxed), 1);
}

#[tokio::test]
async fn retries_are_exhausted() {
let max_retries = 4usize;
let calls = Cell::new(0usize);
let calls = AtomicUsize::new(0);
let result: Result<i32, i32> = execute_with_retry(
max_retries,
TEST_MAX_BACKOFF,
async || {
calls.set(calls.get() + 1);
calls.fetch_add(1, Ordering::Relaxed);
Err(-7)
},
|_error| true,
)
.await;

assert_eq!(result, Err(-7));
assert_eq!(calls.get(), max_retries + 1);
assert_eq!(calls.load(Ordering::Relaxed), max_retries + 1);
}

#[tokio::test]
Expand All @@ -258,10 +263,9 @@ mod tests {
max_retries: 5,
max_backoff: TEST_MAX_BACKOFF,
};
let calls = Cell::new(0usize);
let calls = AtomicUsize::new(0);
let result: Result<i32, Status> = call_with_retry(config, async || {
let attempt = calls.get();
calls.set(attempt + 1);
let attempt = calls.fetch_add(1, Ordering::Relaxed);
if attempt < 2 {
Err(Status::unavailable("connection lost"))
} else {
Expand All @@ -274,7 +278,7 @@ mod tests {
result.expect("call_with_retry should succeed after retriable failures"),
11
);
assert_eq!(calls.get(), 3);
assert_eq!(calls.load(Ordering::Relaxed), 3);
}

#[tokio::test]
Expand All @@ -283,9 +287,9 @@ mod tests {
max_retries: 5,
max_backoff: TEST_MAX_BACKOFF,
};
let calls = Cell::new(0usize);
let calls = AtomicUsize::new(0);
let result: Result<i32, Status> = call_with_retry(config, async || {
calls.set(calls.get() + 1);
calls.fetch_add(1, Ordering::Relaxed);
Err(Status::not_found("missing"))
})
.await;
Expand All @@ -296,7 +300,7 @@ mod tests {
.code(),
Code::NotFound
);
assert_eq!(calls.get(), 1);
assert_eq!(calls.load(Ordering::Relaxed), 1);
}

#[test]
Expand Down
Loading