feat(spider-client): Add user-facing client library for job orchestration and resource group management. - #363
Merged
Merged
Conversation
# Conflicts: # components/spider-proto-rust/src/generated/storage.rs # components/spider-proto/storage/storage.proto # components/spider-scheduler/src/storage_client/grpc.rs
…status_to_error convention
…tus_to_error convention
…der into storage-grpc-migration
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a new Changesspider-client crate
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant SpiderClient
participant JobOrchestrationClient
participant ResourceGroupManagementClient
participant gRPCServer
Caller->>SpiderClient: connect(endpoint, pool_size)
SpiderClient->>JobOrchestrationClient: connect(endpoint, pool_size)
SpiderClient->>ResourceGroupManagementClient: connect(endpoint, pool_size)
JobOrchestrationClient-->>SpiderClient: Ok/Err
ResourceGroupManagementClient-->>SpiderClient: Ok/Err
SpiderClient-->>Caller: Ok(SpiderClient) / Err(ClientError)
Caller->>SpiderClient: submit_job(resource_group_id, task_graph, inputs)
SpiderClient->>JobOrchestrationClient: submit_job(...)
JobOrchestrationClient->>gRPCServer: register_job(RegisterJobRequest)
gRPCServer-->>JobOrchestrationClient: RegisterJobResponse / Status
JobOrchestrationClient-->>SpiderClient: Ok(JobId) / Err(ClientError)
SpiderClient-->>Caller: Ok(JobId) / Err(ClientError)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/error.rs`:
- Around line 26-28: The precondition mapping in the client error handling is
using the job-only ClientError::InvalidJobState variant for
ResourceGroupManagementClient::add_resource_group, which leaks an unrelated
error across the public API. Add a generic/resource-group-specific precondition
error variant in error.rs and update the FAILED_PRECONDITION branch in the
shared RPC helper to return that new variant instead of InvalidJobState so
resource-group failures stay correctly scoped.
In `@components/spider-client/src/job.rs`:
- Around line 112-120: Update the error documentation for both start_job and
cancel_job in job.rs so it fully reflects the errors propagated from
job_state_response_to_result; add ClientError::UnspecifiedJobState and
ClientError::Transport for invalid response states alongside the existing listed
errors. Keep the wording consistent between the two methods and ensure the docs
mention that state conversion can fail independently of the RPC call itself.
- Around line 499-501: The negative-path test currently hardcodes
Endpoint::from_static("http://127.0.0.1:1"), which can be flaky if something is
listening on port 1. Update the test in job.rs to reserve an ephemeral local
port, close/release it before the eager connect, and then build the endpoint
from that freed port so the failure remains deterministic. Use the surrounding
test setup around Endpoint::from_static and the eager connect logic to keep the
change localized.
In `@components/spider-client/src/resource_group.rs`:
- Around line 56-62: Update the public error documentation for both
add_resource_group and verify_resource_group to mention the FAILED_PRECONDITION
mapping coming from resource_group_status_to_error, which currently resolves to
ClientError::InvalidJobState. Keep the docs aligned with the shared mapper
behavior by adding this case to the error list, or adjust the mapper if you
intend a resource-group-specific variant instead.
- Around line 232-234: The negative-path test in resource_group.rs assumes
127.0.0.1:1 is always closed, but that can be environment-dependent. Update the
test setup around Endpoint::from_static to use a closed ephemeral port instead
of hardcoding port 1, so the eager connect failure remains reliable regardless
of local services.
🪄 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: d26b36f1-4ee2-4001-b351-ca6cad66787e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcomponents/spider-client/Cargo.tomlcomponents/spider-client/src/client.rscomponents/spider-client/src/error.rscomponents/spider-client/src/job.rscomponents/spider-client/src/lib.rscomponents/spider-client/src/resource_group.rscomponents/spider-client/src/test_utils.rs
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
Contributor
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/job.rs`:
- Around line 150-152: Update the `cancel_job` documentation to match its actual
error mapping via `job_status_to_error`, since it shares the same behavior as
`start_job`. Add `ClientError::Unauthenticated` to the public error list in the
`cancel_job` doc comment alongside the existing
`ClientError::UnspecifiedJobState` and `ClientError::Transport` entries so the
contract is accurate.
🪄 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: 0976d7a5-df85-470f-8a9a-4255c06a2d46
📒 Files selected for processing (3)
components/spider-client/src/error.rscomponents/spider-client/src/job.rscomponents/spider-client/src/resource_group.rs
💤 Files with no reviewable changes (1)
- components/spider-client/src/resource_group.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- components/spider-client/src/error.rs
LinZhihao-723
requested changes
Jun 29, 2026
Comment on lines
+76
to
+79
| /// | ||
| /// A freshly registered job has no id yet, so the server-reported `NOT_FOUND` and | ||
| /// `FAILED_PRECONDITION` codes (which a job id would otherwise attach) cannot arise for | ||
| /// registration and are folded into [`ClientError::Server`]. |
Member
There was a problem hiding this comment.
Is this over-explanation generated by the coding agent? It doesn't match our doc standard (neither it's format nor it's wording)
Member
There was a problem hiding this comment.
- Might be worth to group
job.rsandresource_group.rsinto agrpcmod. - All these types should probably be public only within the crate.
Comment on lines
+17
to
+21
| /// | ||
| /// Holds a round-robin pool of connections and exposes the resource-group operations (add, verify). | ||
| /// Build one with [`ResourceGroupManagementClient::connect`]. [`crate::client::SpiderClient`] wraps | ||
| /// one of these alongside a [`crate::job::JobOrchestrationClient`] for callers who need both | ||
| /// services behind a single handle. |
Member
There was a problem hiding this comment.
This is also over-explaining: we don't have any of this in the other client implementation.
If you see the agent is generating a wall of text, you should verify whether it's worth.
Comment on lines
+22
to
+26
| /// | ||
| /// Wraps a [`JobOrchestrationClient`] and a [`ResourceGroupManagementClient`] against the same | ||
| /// storage endpoint, so callers who need both job-lifecycle and resource-group operations get a | ||
| /// single handle and one [`SpiderClient::connect`] call. Callers who need only one service may | ||
| /// construct the inner client directly. |
Member
There was a problem hiding this comment.
This is the actual user-facing type. I don't think user needs to know about the underlying client types, right?
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns [`ClientError::Transport`] if tonic cannot establish a connection to `endpoint`. |
Member
There was a problem hiding this comment.
Read this again and tell me if this matches the docstring format, lol
fyi this is also sth already in our codebase and the coding agent just missed it:
| /// | ||
| /// # Errors | ||
| /// | ||
| /// See [`JobOrchestrationClient::submit_job`]. |
Member
There was a problem hiding this comment.
- Shall we use
Forward xxx?? I believe this format was invented by the coding agent. - And in fact, I'm not sure if this is the best way to document the user-facing client.
Member
There was a problem hiding this comment.
I will probably rewrite the docstring in this mod myself since it's user-facing.
But you should really review the docstrings carefully. I don't want to waste more time on catching these types of errors.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
Member
There was a problem hiding this comment.
I've done my editing but please make sure you read what I've changed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/spider-client/src/grpc/job.rs (1)
291-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider unit tests for the status-mapping function.
job_status_to_erroris a pure function, and this exact code shape has already had a real mismapping bug in the siblingresource_group_status_to_error(FAILED_PRECONDITION incorrectly mapped to a job-only variant, caught in a prior review round). A small table-driven test asserting eachCodemaps to the expectedClientErrorvariant would cheaply guard against regressions here and inresource_group.rs.🤖 Prompt for 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. In `@components/spider-client/src/grpc/job.rs` around lines 291 - 300, Add unit tests for the pure status-mapping helper job_status_to_error to prevent regressions in gRPC code-to-ClientError conversion. Create a small table-driven test that exercises each relevant Code branch and asserts the expected ClientError variant, including FailedPrecondition, InvalidArgument, Unauthenticated, Unavailable, NotFound, and the default fallback. Use job_status_to_error as the primary target, and mirror the same approach for resource_group_status_to_error to catch the sibling mapping bug pattern noted in review.
🤖 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 41-43: The doc comment on the `connect` method still references
the wrong error type in its “Returns an error if” section. Update the intra-doc
link from `StorageResponseError::Transport` to `ClientError::Transport`,
matching the other `connect` docs like `SpiderClient::connect` and
`ResourceGroupManagementClient::connect`, so rustdoc can resolve the link
correctly.
---
Nitpick comments:
In `@components/spider-client/src/grpc/job.rs`:
- Around line 291-300: Add unit tests for the pure status-mapping helper
job_status_to_error to prevent regressions in gRPC code-to-ClientError
conversion. Create a small table-driven test that exercises each relevant Code
branch and asserts the expected ClientError variant, including
FailedPrecondition, InvalidArgument, Unauthenticated, Unavailable, NotFound, and
the default fallback. Use job_status_to_error as the primary target, and mirror
the same approach for resource_group_status_to_error to catch the sibling
mapping bug pattern noted in review.
🪄 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: a1f036b5-3ad0-4388-86d9-06e181b7c6dc
📒 Files selected for processing (6)
components/spider-client/src/client.rscomponents/spider-client/src/error.rscomponents/spider-client/src/grpc/job.rscomponents/spider-client/src/grpc/mod.rscomponents/spider-client/src/grpc/resource_group.rscomponents/spider-client/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- components/spider-client/src/client.rs
LinZhihao-723
changed the title
LinZhihao-723
previously approved these changes
Jul 2, 2026
LinZhihao-723
left a comment
LinZhihao-723
left a comment
Member
There was a problem hiding this comment.
Directly modified the PR title.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds client library as a standalone component that expose the job orchestration and resource group management directly.
Note
The resource group deletion is missing, pending fix in storage.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes