feat(spider-huntsman): Complete storage gRPC server: - #356
Closed
sitaowang1998 wants to merge 14 commits into
Closed
feat(spider-huntsman): Complete storage gRPC server:#356sitaowang1998 wants to merge 14 commits into
sitaowang1998 wants to merge 14 commits into
Conversation
Contributor
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe PR updates proto contracts, storage-side gRPC handling, resource-group deletion, execution-manager liveness parsing, and scheduler resend support. It also adds ChangesStorage control-flow updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 4
🧹 Nitpick comments (1)
components/spider-proto/storage/storage.proto (1)
152-165: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReserve the retired protobuf tags before this schema moves forward.
This PR removes wire fields (
session_idplus several response error variants) but does not reserve their numbers/names. That leaves those tags reusable in a later edit, which is how old bytes start decoding as the wrong field.Also applies to: 181-214, 242-244
🤖 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 152 - 165, Reserve the retired protobuf field numbers and names in the affected message definitions in storage.proto so they cannot be reused later. Update the relevant messages around ResourceGroupIdResponse, VerifyResourceGroupRequest, DeleteResourceGroupRequest, and the other referenced response/schema blocks to add reserved tags/names for the removed session_id and deleted error variants, keeping the proto schema forward-safe.
🤖 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-execution-manager/src/client/grpc/liveness.rs`:
- Around line 108-119: The `register_response_to_result` helper currently
accepts a zero `session_id`, unlike `heartbeat_response_to_result`, so add the
same validation there and reject `registration.session_id == 0` with a
`LivenessResponseError::Transport` before building `RegistrationResponse`. Keep
the check alongside the existing `registration` extraction in
`register_response_to_result` so both response paths enforce the same nonzero
session-id rule.
In `@components/spider-scheduler/src/storage_client/grpc.rs`:
- Around line 132-145: The status mapping in map_inbound_status currently treats
every Code::Unavailable as StorageClientError::InboundClosed, which conflates
real queue փակ signals with transient transport failures. Update the match logic
in map_inbound_status to detect the server’s closed marker via status.message()
(for example, a “queue closed” substring) or switch the closed-state RPCs to a
distinct code like FAILED_PRECONDITION, and only return
StorageClientError::InboundClosed for that explicit closed signal. Keep generic
Unavailable errors mapped to StorageClientError::Server so poll_* and
resend_ready_tasks can still retry them.
In `@components/spider-storage/src/grpc.rs`:
- Line 1050: The test fixture in GrpcLivenessClient-related gRPC tests uses
TEST_SESSION_ID with a zero value, but the client contract rejects zero for
registration and heartbeat responses. Update TEST_SESSION_ID to a non-zero
SessionId in grpc.rs so the tests exercise a valid wire value and stay aligned
with the GrpcLivenessClient behavior.
In `@components/spider-storage/src/state/service.rs`:
- Around line 557-568: Serialize delete_resource_group against concurrent
register_job by adding per-resource-group coordination or a tombstone check so
late cache inserts are blocked after deletion. Update delete_resource_group and
the register_job flow around self.inner.db.delete,
self.inner.job_cache.remove_by_resource_group, and job_cache.insert so a
resource group marked deleted cannot be reinserted into cache after the DB row
is removed, and ensure create_task_instance cannot serve work from a deleted
group.
---
Nitpick comments:
In `@components/spider-proto/storage/storage.proto`:
- Around line 152-165: Reserve the retired protobuf field numbers and names in
the affected message definitions in storage.proto so they cannot be reused
later. Update the relevant messages around ResourceGroupIdResponse,
VerifyResourceGroupRequest, DeleteResourceGroupRequest, and the other referenced
response/schema blocks to add reserved tags/names for the removed session_id and
deleted error variants, keeping the proto schema forward-safe.
🪄 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: 0e0fa444-a6da-4970-9717-f38c031556d0
⛔ Files ignored due to path filters (1)
components/spider-proto-rust/src/generated/storage.rsis excluded by!**/generated/**
📒 Files selected for processing (20)
components/spider-execution-manager/src/client/grpc/liveness.rscomponents/spider-execution-manager/src/client/grpc/storage.rscomponents/spider-execution-manager/src/client/storage.rscomponents/spider-execution-manager/src/runtime.rscomponents/spider-proto-rust/src/lib.rscomponents/spider-proto-rust/src/scheduler.rscomponents/spider-proto-rust/src/unpack/storage.rscomponents/spider-proto/storage/storage.protocomponents/spider-scheduler/src/core_impl/round_robin/tests.rscomponents/spider-scheduler/src/error.rscomponents/spider-scheduler/src/storage_client/grpc.rscomponents/spider-scheduler/src/storage_client/mod.rscomponents/spider-storage/src/cache/job.rscomponents/spider-storage/src/db/mariadb.rscomponents/spider-storage/src/grpc.rscomponents/spider-storage/src/state.rscomponents/spider-storage/src/state/job_cache.rscomponents/spider-storage/src/state/service.rscomponents/spider-storage/src/state/test_utils.rscomponents/spider-storage/tests/mariadb_test.rs
💤 Files with no reviewable changes (1)
- components/spider-scheduler/src/error.rs
Comment on lines
+132
to
145
| /// Maps a [`tonic::Status`] returned by an inbound-queue RPC into a [`StorageClientError`]. | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// * [`StorageClientError::InboundClosed`] when the inbound queue is closed, signalled by | ||
| /// `UNAVAILABLE`. | ||
| /// * [`StorageClientError::InvalidInput`] for a malformed request, signalled by `INVALID_ARGUMENT`. | ||
| /// * [`StorageClientError::Server`] for any other failure. | ||
| fn map_inbound_status(status: &tonic::Status) -> StorageClientError { | ||
| match status.code() { | ||
| Code::Unavailable => StorageClientError::InboundClosed, | ||
| Code::InvalidArgument => to_invalid_input_error(status.message()), | ||
| _ => StorageClientError::Server(status.message().to_owned()), | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Don't treat every UNAVAILABLE as InboundClosed
tonic::Status uses Code::Unavailable for both server-side queue closures and transient transport/network failures. Mapping all Unavailable errors to StorageClientError::InboundClosed causes the scheduler to incorrectly treat temporary outages as permanent lane closures across poll_* and resend_ready_tasks.
Distinguish the server's closed signal from client-side Unavailable errors by checking status.message() (e.g., for a "queue closed" substring) or using a distinct gRPC code like FAILED_PRECONDITION for the closed state. Transient Unavailable errors should remain on the transport path to allow for retries.
🤖 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-scheduler/src/storage_client/grpc.rs` around lines 132 -
145, The status mapping in map_inbound_status currently treats every
Code::Unavailable as StorageClientError::InboundClosed, which conflates real
queue փակ signals with transient transport failures. Update the match logic in
map_inbound_status to detect the server’s closed marker via status.message()
(for example, a “queue closed” substring) or switch the closed-state RPCs to a
distinct code like FAILED_PRECONDITION, and only return
StorageClientError::InboundClosed for that explicit closed signal. Keep generic
Unavailable errors mapped to StorageClientError::Server so poll_* and
resend_ready_tasks can still retry them.
Comment on lines
+557
to
+568
| pub async fn delete_resource_group( | ||
| &self, | ||
| resource_group_id: ResourceGroupId, | ||
| password: &[u8], | ||
| ) -> Result<(), StorageServerError> { | ||
| self.inner.db.verify(resource_group_id, password).await?; | ||
| self.inner.db.delete(resource_group_id).await?; | ||
| let evicted_jobs = self | ||
| .inner | ||
| .job_cache | ||
| .remove_by_resource_group(resource_group_id) | ||
| .await; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize deletion against in-flight job registration.
register_job writes the DB row before it inserts the JCB into cache. If a registration commits before Line 563 and only reaches job_cache.insert(...) after Line 568, this method returns success with the MariaDB rows gone but a live cached job still present. create_task_instance then continues serving work from cache for a resource group that was already deleted.
Please add per-resource-group coordination or a tombstone/check that prevents late cache insertion after deletion.
🤖 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 557 - 568,
Serialize delete_resource_group against concurrent register_job by adding
per-resource-group coordination or a tombstone check so late cache inserts are
blocked after deletion. Update delete_resource_group and the register_job flow
around self.inner.db.delete, self.inner.job_cache.remove_by_resource_group, and
job_cache.insert so a resource group marked deleted cannot be reinserted into
cache after the DB row is removed, and ensure create_task_instance cannot serve
work from a deleted group.
Collaborator
Author
|
Closed as too large. Will split into separate pr. |
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
1 participant
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.
ResendReadyTasksRPC to a dedicated scheduler-facing service #352 by adding backResendReadyTasksandDeleteResourceGroup.Description
Use gRPC status instead of custom error code
Replace remaining error codes in gRPC protocol (
InboundQueueResponseError,ResourceGroupManagementError,ExecutionManagerLivenessError,SchedulerRegistrationError) with status.Update scheduler and execution manager clients to parse gRPC status into local error type.
Add delete resource group implementation
Adds resource group deletion in database and in cache. This also removes all jobs belonging to this resource group from both database and cache.
Warning
Because of the limitation in the scheduler queue implementation, enqueued tasks owned by the resource group are not removed. However, the scheduled task should later fail at task instance creation.
There is known possible race condition between the deletion and another task instance creation. However, the task should fail later when reporting result, with no extra action needed by storage.
Add
ResendReadyTasksAs pointed out in #352,
ResendReadyTasksis now put intoInboundQueueService.Checklist
breaking change.
Validation performed
Summary by CodeRabbit