feat(huntsman): Add the skeleton for gRPC storage server. - #346
Conversation
|
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:
WalkthroughThe PR adds Changestask_instance_id Propagation Through Reporting Flow
New spider-storage gRPC Server Binary and Service Adapter
Sequence DiagramsequenceDiagram
participant main as main()
participant ServerConfig as ServerConfig
participant Runtime as Runtime
participant GrpcServiceState as GrpcServiceState
participant TonicServer as tonic::Server
main->>ServerConfig: from_yaml_file(config)
ServerConfig-->>main: ServerConfig { host, port, runtime }
main->>Runtime: create_runtime(server_config.runtime)
Runtime-->>main: (runtime, service_state)
main->>GrpcServiceState: new(service_state)
GrpcServiceState-->>main: grpc_service_state
main->>TonicServer: add_service ×6 (cloned state)
main->>TonicServer: serve_with_shutdown(addr, Ctrl-C)
note over TonicServer: Serving gRPC requests
TonicServer-->>main: shutdown on Ctrl-C
main->>Runtime: stop()
Runtime-->>main: Ok(())
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🧹 Nitpick comments (1)
components/spider-storage/src/state/service.rs (1)
545-554: 💤 Low valueConsider adding logging for operational visibility.
add_resource_group(line 512-514) logs on success, but this delete method does not. For audit trails and operational debugging, logging resource group deletions would be helpful.♻️ Suggested improvement
pub async fn delete_resource_group( &self, resource_group_id: ResourceGroupId, ) -> Result<(), StorageServerError> { self.inner .db .delete(resource_group_id) - .await - .map_err(StorageServerError::from) + .await?; + tracing::info!( + rg_id = ? resource_group_id, + "Resource group deleted.", + ); + Ok(()) }🤖 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-storage/src/state/service.rs` around lines 545 - 554, The delete_resource_group method lacks logging for audit trails and operational visibility, unlike the add_resource_group method which logs on success. Add a logging statement to the delete_resource_group method to record successful resource group deletions, including the resource_group_id for context. This should be added after the successful completion of the database delete operation to maintain consistency with the logging approach used in add_resource_group.
🤖 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-storage/src/bin/grpc_server.rs`:
- Around line 39-59: The doc comments for the CLI argument fields (db_host,
db_port, db_name, db_username, db_password, and related fields) are failing
rustdoc markdown lint checks because identifier-like terms are not wrapped in
backticks. Fix this by wrapping all identifier-like terms such as "MariaDB" and
other configuration-related terms in backticks (e.g., `MariaDB`) within the doc
comment strings. Apply this to all affected field documentation comments to
satisfy the lint requirements.
---
Nitpick comments:
In `@components/spider-storage/src/state/service.rs`:
- Around line 545-554: The delete_resource_group method lacks logging for audit
trails and operational visibility, unlike the add_resource_group method which
logs on success. Add a logging statement to the delete_resource_group method to
record successful resource group deletions, including the resource_group_id for
context. This should be added after the successful completion of the database
delete operation to maintain consistency with the logging approach used in
add_resource_group.
🪄 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: 44478ae4-b403-40b5-b30c-6445f84464eb
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcomponents/spider-proto-rust/src/generated/storage.rsis excluded by!**/generated/**
📒 Files selected for processing (11)
components/spider-execution-manager/src/client/grpc/storage.rscomponents/spider-execution-manager/src/client/storage.rscomponents/spider-execution-manager/src/runtime.rscomponents/spider-proto/storage/storage.protocomponents/spider-storage/Cargo.tomlcomponents/spider-storage/src/bin/grpc_server.rscomponents/spider-storage/src/grpc.rscomponents/spider-storage/src/lib.rscomponents/spider-storage/src/state/service.rstests/huntsman/em-runtime/tests/test_runtime.rstests/huntsman/test-utils/src/mock.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/spider-storage/src/grpc.rs (1)
107-113: 💤 Low valueConsider renaming
input_counttoinputs_sizefor consistency.The field
task_graph_sizecorrectly conveys byte count, butinput_countsuggests a number of items when it's actuallyserialized_inputs.len()(byte count). Considerinputs_sizeorserialized_inputs_lento avoid confusion when analyzing logs.Suggested fix
tracing::debug!( session_id = request.session_id, resource_group_id = request.resource_group_id, task_graph_size = request.serialized_task_graph.len(), - input_count = request.serialized_inputs.len(), + inputs_size = request.serialized_inputs.len(), "Received SubmitJob request." );🤖 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-storage/src/grpc.rs` around lines 107 - 113, The field name `input_count` in the tracing::debug! macro is misleading because it represents the byte length of serialized_inputs (via .len()), not a count of input items. Rename the field from `input_count` to `inputs_size` in the tracing::debug! call to accurately convey that it's a byte size measurement, making it consistent with the `task_graph_size` field naming convention.
🤖 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.
Nitpick comments:
In `@components/spider-storage/src/grpc.rs`:
- Around line 107-113: The field name `input_count` in the tracing::debug! macro
is misleading because it represents the byte length of serialized_inputs (via
.len()), not a count of input items. Rename the field from `input_count` to
`inputs_size` in the tracing::debug! call to accurately convey that it's a byte
size measurement, making it consistent with the `task_graph_size` field naming
convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5b20aaef-0802-4080-a5f7-b34743988c1a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
components/spider-storage/src/bin/grpc_server.rscomponents/spider-storage/src/grpc.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- components/spider-storage/src/bin/grpc_server.rs
# Conflicts: # components/spider-execution-manager/src/runtime.rs # tests/huntsman/em-runtime/tests/test_runtime.rs
LinZhihao-723
left a comment
There was a problem hiding this comment.
Partial review results for the bin entrypoint:
- Instead of exposing everything as cli arguments and reconstruct the runtime config, we should probably start with CLP-package style configuration: we make the runtime config serde-compatible, with all subconfig being the fields inside
RuntimeConfig. We will need an extra level ofServerConfig, containing the details of where the gprc server will be hosted. The cli should only parser the path of this config to start the service.- Pending: we may need to discuss with the cloud team to understand their current way for passing the config to the hosting container.
- For now, the DB credentials are still passed explicitly as parameters. We might need to use environment variables as what CLP-package does.
- The logger is not properly configured: it's missing the basic setup and the unblocking writer. Made a helper for it (and we probably want to make a dedicated crate for the bin helpers outside of
spider-corelib, maybe calledspider-utils, to make the helper reusable for other bins).
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-storage/src/config.rs`:
- Around line 25-42: The error documentation in the from_yaml_file method is
incomplete because it only mentions yaml_serde::from_reader as a potential error
source, but the method can also fail when calling File::open on line 38. Update
the documentation comment for the from_yaml_file method to explicitly document
both potential failure points: File::open (which can return an io::Error) and
yaml_serde::from_reader. Make sure the Errors section clearly lists both sources
that can cause the function to return an error.
🪄 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: 7ca248f9-4bc4-4cbd-8f03-65f9e4cef64e
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcomponents/spider-proto-rust/src/generated/storage.rsis excluded by!**/generated/**
📒 Files selected for processing (15)
components/spider-execution-manager/src/runtime.rscomponents/spider-proto/storage/storage.protocomponents/spider-storage/Cargo.tomlcomponents/spider-storage/src/bin/grpc_server.rscomponents/spider-storage/src/config.rscomponents/spider-storage/src/grpc.rscomponents/spider-storage/src/lib.rscomponents/spider-storage/src/logging.rscomponents/spider-storage/src/ready_queue.rscomponents/spider-storage/src/state/job_cache_gc.rscomponents/spider-storage/src/state/runtime.rscomponents/spider-storage/src/state/service.rscomponents/spider-storage/src/task_instance_pool.rstests/huntsman/em-runtime/tests/test_runtime.rstests/huntsman/test-utils/src/mock.rs
✅ Files skipped from review due to trivial changes (2)
- components/spider-storage/src/logging.rs
- tests/huntsman/em-runtime/tests/test_runtime.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- components/spider-storage/src/state/service.rs
- components/spider-storage/Cargo.toml
- tests/huntsman/test-utils/src/mock.rs
- components/spider-proto/storage/storage.proto
- components/spider-execution-manager/src/runtime.rs
- components/spider-storage/src/grpc.rs
| impl ServerConfig { | ||
| /// Loads a [`ServerConfig`] from the YAML file at the given path. | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// The parsed [`ServerConfig`] on success. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if: | ||
| /// | ||
| /// * Forwards [`yaml_serde::from_reader`]'s return values on failure. | ||
| pub fn from_yaml_file(path: &Path) -> Result<Self, ConfigError> { | ||
| let file = File::open(path)?; | ||
| let config = yaml_serde::from_reader(file)?; | ||
| Ok(config) | ||
| } | ||
| } |
There was a problem hiding this comment.
Error documentation is incomplete.
The documentation on lines 34-36 states "Forwards [yaml_serde::from_reader]'s return values on failure" but from_yaml_file can also fail during File::open (line 38), which returns an io::Error. The docs should mention both potential error sources.
📝 Suggested documentation fix
/// # Errors
///
/// Returns an error if:
///
- /// * Forwards [`yaml_serde::from_reader`]'s return values on failure.
+ /// * The file cannot be opened.
+ /// * The YAML content cannot be deserialized into a [`ServerConfig`].
pub fn from_yaml_file(path: &Path) -> Result<Self, ConfigError> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl ServerConfig { | |
| /// Loads a [`ServerConfig`] from the YAML file at the given path. | |
| /// | |
| /// # Returns | |
| /// | |
| /// The parsed [`ServerConfig`] on success. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Returns an error if: | |
| /// | |
| /// * Forwards [`yaml_serde::from_reader`]'s return values on failure. | |
| pub fn from_yaml_file(path: &Path) -> Result<Self, ConfigError> { | |
| let file = File::open(path)?; | |
| let config = yaml_serde::from_reader(file)?; | |
| Ok(config) | |
| } | |
| } | |
| impl ServerConfig { | |
| /// Loads a [`ServerConfig`] from the YAML file at the given path. | |
| /// | |
| /// # Returns | |
| /// | |
| /// The parsed [`ServerConfig`] on success. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Returns an error if: | |
| /// | |
| /// * The file cannot be opened. | |
| /// * The YAML content cannot be deserialized into a [`ServerConfig`]. | |
| pub fn from_yaml_file(path: &Path) -> Result<Self, ConfigError> { | |
| let file = File::open(path)?; | |
| let config = yaml_serde::from_reader(file)?; | |
| Ok(config) | |
| } | |
| } |
🤖 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-storage/src/config.rs` around lines 25 - 42, The error
documentation in the from_yaml_file method is incomplete because it only
mentions yaml_serde::from_reader as a potential error source, but the method can
also fail when calling File::open on line 38. Update the documentation comment
for the from_yaml_file method to explicitly document both potential failure
points: File::open (which can return an io::Error) and yaml_serde::from_reader.
Make sure the Errors section clearly lists both sources that can cause the
function to return an error.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/spider-storage/src/grpc.rs (1)
694-702: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
BadRequestfor invalid scheduler registrations.Invalid IP/port values are converted to
StorageServerError::BadRequest, but the scheduler error helper currently reports scheduler errors as server errors. Add a scheduler-registration error-code mapper so client input failures are returned asBadRequest, matching the other service domains.Also applies to: 718-719, 1147-1159
🤖 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-storage/src/grpc.rs` around lines 694 - 702, The scheduler registration error handling is converting client input failures to server errors instead of preserving them as BadRequest errors. Create a scheduler-registration error-code mapper function that properly converts client input validation errors (such as invalid IP addresses or ports) to StorageServerError::BadRequest, matching the pattern used in other service domains. Apply this mapper to handle scheduler registration errors at all relevant locations (the current code block around lines 694-702, and also the similar error handling at lines 718-719 and 1147-1159) so that invalid client input is consistently returned as BadRequest rather than server errors.
🧹 Nitpick comments (1)
components/spider-storage/src/grpc.rs (1)
460-463: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKeep poll request logs below
info.These polling RPCs can run continuously even when no work is returned. Downgrade the success-path “request received” logs to
debugortrace; keep theinspect_errlogs aterror.Suggested logging-level adjustment
- tracing::info!( + tracing::debug!( max_items = request.max_items, wait_ms = request.wait_ms, "Ready tasks poll request received." );Apply the same change to the commit and cleanup poll request logs.
Also applies to: 483-486, 506-509
🤖 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-storage/src/grpc.rs` around lines 460 - 463, The polling RPC request received logs are currently at the info level, which can produce excessive noise from continuous polling. Downgrade the tracing::info! macro calls for the "Ready tasks poll request received" message to tracing::debug! or tracing::trace! to reduce log verbosity. Apply this same change to all similar polling request received logs throughout the file, including the commit and cleanup poll request logs, while keeping any inspect_err logs at the error level.
🤖 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.
Outside diff comments:
In `@components/spider-storage/src/grpc.rs`:
- Around line 694-702: The scheduler registration error handling is converting
client input failures to server errors instead of preserving them as BadRequest
errors. Create a scheduler-registration error-code mapper function that properly
converts client input validation errors (such as invalid IP addresses or ports)
to StorageServerError::BadRequest, matching the pattern used in other service
domains. Apply this mapper to handle scheduler registration errors at all
relevant locations (the current code block around lines 694-702, and also the
similar error handling at lines 718-719 and 1147-1159) so that invalid client
input is consistently returned as BadRequest rather than server errors.
---
Nitpick comments:
In `@components/spider-storage/src/grpc.rs`:
- Around line 460-463: The polling RPC request received logs are currently at
the info level, which can produce excessive noise from continuous polling.
Downgrade the tracing::info! macro calls for the "Ready tasks poll request
received" message to tracing::debug! or tracing::trace! to reduce log verbosity.
Apply this same change to all similar polling request received logs throughout
the file, including the commit and cleanup poll request logs, while keeping any
inspect_err logs at the error level.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f840dfbb-5211-4df1-a9c9-19e09303f608
⛔ Files ignored due to path filters (1)
components/spider-proto-rust/src/generated/storage.rsis excluded by!**/generated/**
📒 Files selected for processing (4)
components/spider-proto/storage/storage.protocomponents/spider-storage/src/bin/grpc_server.rscomponents/spider-storage/src/grpc.rscomponents/spider-storage/src/state/runtime.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- components/spider-storage/src/bin/grpc_server.rs
- components/spider-storage/src/state/runtime.rs
LinZhihao-723
left a comment
There was a problem hiding this comment.
Changes:
- Removed session ID from job submission request: the user-facing API doesn't need to expose the session ID.
- As discussed offline, the previous implementation has messy error handling and logging setup. I have removed the service implementation and will resume it in the next few PRs.
- Move
loggingto a new cratespider-utilsfor sharing across different components. - Make the service listen to the cancellation token: if the token is cancelled, we should stop the service. This allows the error handler to fire the cancellation token on internal errors and thus gracefully terminate the runtime.
Since we removed the service implementation, for the PR title, how about:
feat(huntsman): Add the skeleton for gRPC storage server.
| rpc GetJobError(JobIdRequest) returns (JobErrorResponse); | ||
| rpc DeleteExpiredTerminatedJobs(DeleteExpiredTerminatedJobsRequest) | ||
| returns (DeleteExpiredTerminatedJobsResponse); | ||
| rpc ResendReadyTasks(ResendReadyTasksRequest) returns (JobManagementOperationResponse); |
There was a problem hiding this comment.
@CodeRabbit ResendReadyTask should not be delete, same as the DeleteResourceGroup below. However, we might need to put ResendReadyTask in another trait later, as it is used by scheduler, while other functions in this trait are used by client.
There was a problem hiding this comment.
@sitaowang1998, understood — ResendReadyTasks and DeleteResourceGroup should be restored to the proto. The design note is also well-taken: since ResendReadyTasks is consumed by the scheduler rather than clients, it would make sense to eventually move it to a scheduler-specific service to keep the caller boundaries clean.
Would you like me to open a follow-up GitHub issue to track moving ResendReadyTasks into a dedicated scheduler-facing service?
✏️ Learnings added
Learnt from: sitaowang1998
Repo: y-scope/spider PR: 346
File: components/spider-proto/storage/storage.proto:14-14
Timestamp: 2026-06-23T15:43:50.235Z
Learning: In `components/spider-proto/storage/storage.proto`, the `ResendReadyTasks` RPC in `JobOrchestrationService` must NOT be removed; it is actively used by the scheduler. A future refactor may move it to a separate scheduler-facing service, since the other RPCs in `JobOrchestrationService` are used by clients. Similarly, `DeleteResourceGroup` in `ResourceGroupManagementService` must not be removed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Please open an issue to track this.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@CodeRabbit Please open an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
gh issue create \
--repo "y-scope/spider" \
--title "Move \`ResendReadyTasks\` RPC to a dedicated scheduler-facing service" \
--body "## Summary
The \`ResendReadyTasks\` RPC currently lives in \`JobOrchestrationService\` inside \`components/spider-proto/storage/storage.proto\`. However, this RPC is consumed by the **scheduler**, whereas the other RPCs in \`JobOrchestrationService\` are used by **clients**. Mixing caller audiences in a single service makes the API boundary unclear.
## Required Changes
- Move the \`ResendReadyTasks\` RPC (and its associated request message, currently \`ResendReadyTasksRequest\`) out of \`JobOrchestrationService\` and into a new scheduler-facing service (e.g., \`SchedulerOrchestrationService\` or similar) in \`components/spider-proto/storage/storage.proto\`.
- Update the corresponding server-side handler in \`components/spider-storage/src/grpc.rs\` to implement the new service trait generated by tonic.
- Ensure any scheduler-side gRPC client code is updated to call the new service endpoint.
## Rationale
Separating scheduler-only RPCs from client-facing RPCs keeps service boundaries clean, makes authorization/middleware easier to apply per-caller type, and improves API clarity.
## Affected Areas
- \`components/spider-proto/storage/storage.proto\`
- \`components/spider-storage/src/grpc.rs\`
- Scheduler gRPC client code (wherever \`ResendReadyTasks\` is called)
## References
- PR: https://github.com/y-scope/spider/pull/346
- Comment: https://github.com/y-scope/spider/pull/346#discussion_r3461022333
/cc `@sitaowang1998`" \
--assignee "sitaowang1998"Length of output: 1833
@sitaowang1998, the follow-up issue has been created and assigned to you: #352 — "Move ResendReadyTasks RPC to a dedicated scheduler-facing service".
Description
This PR:
task_instance_idto task result and error report gRPC protocol.Warning
The resource group deletion is not implemented in the MariaDB storage layer.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Removals
DeleteExpiredTerminatedJobsandResendReadyTasksendpoints.DeleteResourceGroupendpoint.