feat(spider-client)!: Retry gRPC connection failures with a configurable exponential-backoff policy. - #396
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughChangesSpider client construction now uses a configurable builder with pool and retry settings. Shared retry utilities handle transient gRPC failures with bounded backoff, and job/resource-group RPCs use them. The e2e test driver adopts the builder API. Spider client retry integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant SpiderClientBuilder
participant JobOrchestrationClient
participant ResourceGroupManagementClient
participant call_with_retry
participant gRPCService
Caller->>SpiderClientBuilder: configure and connect
SpiderClientBuilder->>JobOrchestrationClient: create pool with RetryConfig
SpiderClientBuilder->>ResourceGroupManagementClient: create pool with RetryConfig
Caller->>JobOrchestrationClient: invoke job RPC
JobOrchestrationClient->>call_with_retry: execute RPC
call_with_retry->>gRPCService: retry unavailable calls
Caller->>ResourceGroupManagementClient: invoke resource-group RPC
ResourceGroupManagementClient->>call_with_retry: execute RPC
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/spider-client/src/grpc/job.rs`:
- Around line 93-101: Remove the call_with_retry wrapper around register_job in
the job submission flow, invoking the RPC once while preserving the existing
status mapping and into_inner handling. Keep retry behavior available only for
idempotent RPCs, and do not add retries without an idempotency key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2c69add3-d223-4cfd-a082-4571585aa8e2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
components/spider-client/src/client.rscomponents/spider-client/src/grpc/job.rscomponents/spider-client/src/grpc/resource_group.rscomponents/spider-client/src/lib.rscomponents/spider-utils/Cargo.tomlcomponents/spider-utils/src/grpc/mod.rscomponents/spider-utils/src/grpc/retry.rstests/huntsman/e2e/src/test_driver.rs
|
tonic sometimes map |
I will do it. In the meanwhile, can you check what exactly the error is when the status is |
|
sitaowang1998
left a comment
There was a problem hiding this comment.
Verified that the e2e test pass with a temporary fix for storage enqueue discussed offline.
Description
This PR makes
SpiderClientretry transient gRPC failures instead of surfacing the first lost-connection error to the caller. The retry logic is a reusable async helper added tospider-utils, the tonic-specific "which failures are retriable" policy lives alongside it, andSpiderClientgains a builder so callers can tune the retry behavior (and the existing connection-pool size) with named, optional settings. Every gRPC round-trip the client issues is now wrapped in this retry policy.Because retry needs a call the helper can invoke repeatedly, the wrapped closure re-pulls a client from the connection pool on each attempt, so a retry is re-sent over the pool's next round-robin connection rather than the one that just failed.
Generic retry helper (
spider-utils)spider_utils::grpc::retry::execute_with_retry(max_retries, max_backoff, grpc_call, is_retriable): an async helper generic over the success and error types, the async call closure (AsyncFnMut() -> Result<ResponseType, ErrorType>), and anis_retriable: Fn(&ErrorType) -> boolclassifier. It returns as soon as the call succeeds, returns immediately when the error is classified non-retriable, and otherwise retries until the retry budget is exhausted and returns the last error.max_retriescounts attempts after the initial one, so the call is invoked at mostmax_retries + 1times.max_backoff, plus a small uniform random jitter (up to 20ms) so concurrent clients don't retry in lockstep. The doubling is overflow-safe and the jitter is additive on top of the cap, so an actual wait may slightly exceedmax_backoff.RetryConfig { max_retries: usize, max_backoff: Duration }(Clone,Copy,Debug), a small bundle of the two knobs with aDefaultof 10 retries and a 3s backoff cap.call_with_retry(retry_config, grpc_call), a thin tonic-facing wrapper overexecute_with_retrythat supplies the retriable policy: a gRPCStatusis retried only when its code isUNAVAILABLE(a lost or unestablished connection). All other status codes are treated as deterministic and returned immediately.Configurable client via a builder (
spider-client)SpiderClientBuilderandSpiderClient::builder(endpoint). The builder exposes consuming,#[must_use]setters —pool_size,max_retries,max_backoff— and an asyncconnect()that establishes the pools and returns aSpiderClient. Unset knobs fall back to a default pool size of 8 andRetryConfig::default().SpiderClient::connect(endpoint, pool_size)constructor; the builder is now the sole entry point. Migration isSpiderClient::builder(endpoint).pool_size(pool_size).connect().await.SpiderClientBuilderandRetryConfigfrom the crate root so callers configuring retries don't need to reach intospider-utils.Retry wiring in the gRPC clients (
spider-client)JobOrchestrationClient,ResourceGroupManagementClient) now carry aRetryConfig, threaded from the builder through theirconnectconstructors.submit_job,start_job,cancel_job,get_job_state,get_job_outputs,get_job_error) and the two resource-group calls (add_resource_group,verify_resource_group) — now issue their gRPC round-trip throughcall_with_retry. Only the network call is inside the retried closure; request serialization/compression and response deserialization stay outside it and run exactly once. The per-attempt request is cloned so each retry re-sends over a freshly selected pooled connection.Caller update (huntsman e2e)
SpiderClient::connect(endpoint, concurrency)toSpiderClient::builder(endpoint).pool_size(concurrency).connect().await, and fixes the corresponding docstring reference.Tests
spider-utils: success on the first attempt, success after a run of retriable failures, immediate return on a non-retriable error, retry-budget exhaustion (asserting exactlymax_retries + 1invocations), and a bounds test asserting the jittered backoff always lands within[capped, capped + jitter]across the exponential and clamped regions.call_with_retry: anUNAVAILABLEstatus is retried and then succeeds, and a non-retriable status (NOT_FOUND) returns immediately without retrying.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes
Tests