feat(huntsman): Complete storage gRPC protocol and its clients: - #340
Conversation
WalkthroughAdds domain-scoped storage proto services/enums, bidirectional protobuf↔core conversions, a GrpcLivenessClient, updates execution-manager storage adapter to task-instance types, and introduces a GrpcSchedulerStorageClient with scheduler wiring and public re-exports. ChangesProto, gRPC clients, and conversions
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/spider-proto/storage/storage.proto (1)
1-352: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdd comprehensive documentation to the proto API.
The proto file defines multiple public gRPC services and complex message structures but contains zero documentation comments. For a cross-component API contract, comprehensive documentation is essential to ensure maintainability, enable safe evolution, and support developer onboarding.
Please add proto comments documenting:
- Each service's purpose and usage context
- Each RPC's behaviour, preconditions, and error scenarios
- Message field semantics, constraints, and valid ranges
- Error code meanings and client handling expectations
- The JobState enum transitions and their lifecycle implications
🤖 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-proto/storage/storage.proto` around lines 1 - 352, The proto file lacks any documentation; add clear proto comments for all public symbols: each service (JobManagementService, TaskInstanceManagementService, SchedulerStorageService, ResourceGroupManagementService, ExecutionManagerLivenessService, SessionManagementService) describing purpose and usage; each RPC (e.g., SubmitJob, StartJob, PollReadyTasks, RegisterTaskInstance, ReportTaskSuccess, AddResourceGroup, RegisterExecutionManager, GetSession) explaining input preconditions, side effects, success/failure semantics and typical error scenarios; each message and important fields (SubmitJobRequest.serialized_task_graph, ReadyTask.task_id, RegisterTaskInstanceRequest.execution_manager_id, etc.) documenting field semantics, valid ranges and constraints; all error messages (JobManagementError.ErrCode, TaskInstanceError.ErrCode, SchedulerStorageError.ErrCode, ResourceGroupError.ErrCode, ExecutionManagerLivenessError.ErrCode) describing when each code is returned and recommended client handling; and the JobState enum documenting legal state transitions (e.g., READY -> RUNNING -> COMMIT_READY/CLEANUP_READY -> SUCCEEDED/FAILED/CANCELLED) and lifecycle implications. Keep comments concise, use proto // or /** */ comments above definitions so they are included in generated docs and ensure examples or notes for session_id and storage_session consistency where relevant.
🤖 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-proto/storage/storage.proto`:
- Around line 1-352: The proto file lacks any documentation; add clear proto
comments for all public symbols: each service (JobManagementService,
TaskInstanceManagementService, SchedulerStorageService,
ResourceGroupManagementService, ExecutionManagerLivenessService,
SessionManagementService) describing purpose and usage; each RPC (e.g.,
SubmitJob, StartJob, PollReadyTasks, RegisterTaskInstance, ReportTaskSuccess,
AddResourceGroup, RegisterExecutionManager, GetSession) explaining input
preconditions, side effects, success/failure semantics and typical error
scenarios; each message and important fields
(SubmitJobRequest.serialized_task_graph, ReadyTask.task_id,
RegisterTaskInstanceRequest.execution_manager_id, etc.) documenting field
semantics, valid ranges and constraints; all error messages
(JobManagementError.ErrCode, TaskInstanceError.ErrCode,
SchedulerStorageError.ErrCode, ResourceGroupError.ErrCode,
ExecutionManagerLivenessError.ErrCode) describing when each code is returned and
recommended client handling; and the JobState enum documenting legal state
transitions (e.g., READY -> RUNNING -> COMMIT_READY/CLEANUP_READY ->
SUCCEEDED/FAILED/CANCELLED) and lifecycle implications. Keep comments concise,
use proto // or /** */ comments above definitions so they are included in
generated docs and ensure examples or notes for session_id and storage_session
consistency where relevant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a9894bac-13ee-4fdd-8177-6fede9a84856
⛔ 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 (8)
components/spider-execution-manager/src/client/grpc/liveness.rscomponents/spider-execution-manager/src/client/grpc/storage.rscomponents/spider-proto/storage/storage.protocomponents/spider-scheduler/Cargo.tomlcomponents/spider-scheduler/src/error.rscomponents/spider-scheduler/src/grpc/mod.rscomponents/spider-scheduler/src/grpc/storage_client.rscomponents/spider-scheduler/src/lib.rs
✅ Files skipped from review due to trivial changes (2)
- components/spider-scheduler/Cargo.toml
- components/spider-scheduler/src/grpc/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- components/spider-execution-manager/src/client/grpc/liveness.rs
There was a problem hiding this comment.
Reviewed all changes and applied the following code-level changes:
proto
JobManagementService->JobOrchestrationServiceto match the protocol trait's nameJobManagementError->JobOrchestrationError
TaskInstanceError->TaskInstanceManagementError.TaskInstanceErrorsounds like the error inside the task instance itself.ResourceGroupError->ResourceGroupManagementError.serialized_inputsshould bebytes, notrepeated bytes. The inputs should be submitted in a serialized form, which allows the client and server to determine how the bytes should be decoded.SchedulerStorageService->InboundQueueServiceto match the term we use in the scheduler.SchedulerStorageError->InboundQueueResponseError
- Remove
IsExecutionManagerAliveandGetDeadExecutionManagersfromExecutionManagerLivenessService: these operations are internal to the storage service, we don't need gRPC methods for them. - Change
PollReadyTaskRequest'swait_nstowait_ms: None of our clients use nanoseconds.
spider-proto-rust
- Create a dedicated error type under
error.rsmod, export it through the library layer. Use this error type forid.rsandjob.rsto replace the use of string errors.- Client implementation should be changed accordingly.
spider-execution-manager
storage_error_to_liveness_errorshould be renamed to reflect the error type renaming.- Use
impl From<storage::ExecutionManagerLivenessError> for LivenessResponseErrorto replace the use ofstorage_error_to_liveness_error. This was done in the storage client in the previous PR. - Use constants to replace magic numbers/strings in the test cases.
spider-scheduler
- Restructure the client mod in the following structure (to match the execution manager trait):
- storage_client
- mod.rs (containing the client trait)
- grpc.rs (containing the gPRC client implementation)
- storage_client
- Drop
inbound_queue_response_error_to_client_error. - Doesn't make sense to return
Transporterror for request out-of-range inpoll_ready_task_request. UseInvalidInputinstead. - Docstrings are not inconsistent with other
_to_resultmethods inspider-execution-manager. - The session tracker doesn't make any sense: no methods will update it, and used for all requests. If we look at the storage service in the dev branch, retrieving the job state doesn't need the session ID. Thus, the session ID is dropped from the proto source; the session tracker is removed from the client.
- Use constants to replace magic numbers/strings in the test cases.
Besides some naming issues, I need to point out that some changes above are not supposed to be my job for reviewing this PR:
- Considering we already have a gRPC PR reviewed and merged, I'd assume you should read my changes and make sure these changes are propagated to the consequent PRs. But part of my editing of this review is still repeating what I've done in the last PR.
- Some of the code generated by the coding agent doesn't make sense. You probably need to spend more time reviewing them yourself.
Please review my changes and be careful with the pending gRPC PR for the scheduler-em communication.
LinZhihao-723
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(huntsman): Complete storage gRPC protocol and its clients
* Add protobuf definitions for the storage gRPC protocol.
* Implement the storage client for `spider-scheduler`.
* Implement the liveness client for `spider-execution-manager`.
spider-scheduler.spider-execution-manager.Description
This PR:
spider-core.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Error Handling
Tests