fix(huntsman): Add ResendReadyTasks protocol, implmentation and usage. - #372
Closed
sitaowang1998 wants to merge 36 commits into
Closed
fix(huntsman): Add ResendReadyTasks protocol, implmentation and usage.#372sitaowang1998 wants to merge 36 commits into
ResendReadyTasks protocol, implmentation and usage.#372sitaowang1998 wants to merge 36 commits into
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
…o storage-grpc-services
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a storage RPC for resending ready tasks, wires it through the scheduler client and runtime, implements storage gRPC endpoints, centralizes error handling, adds scheduler registration conversion, and extends request unpacking. ChangesResend ready tasks and scheduler wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime as create_runtime
participant Client as GrpcSchedulerStorageClient
participant Storage as InboundQueueService
Runtime->>Client: register_scheduler()
Client->>Storage: gRPC RegisterScheduler
Storage-->>Client: RegisterSchedulerResponse
Runtime->>Client: resend_ready_tasks()
Client->>Storage: gRPC ResendReadyTasks(Void)
Storage-->>Client: Void response
Client-->>Runtime: Ok(())
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
🧹 Nitpick comments (2)
components/spider-scheduler/src/runtime.rs (1)
128-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging on successful resend, matching the registration log above.
Line 126 logs after a successful
register, but the newresend_ready_taskscall is silent on success, making it harder to confirm in logs that ready-task recovery ran after a scheduler restart.♻️ Suggested tweak
storage_client.resend_ready_tasks().await?; + tracing::info!("Resent ready tasks to storage.");🤖 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/runtime.rs` around lines 128 - 129, The resend_ready_tasks recovery path in runtime::run is currently silent on success, unlike the nearby register success log. After storage_client.resend_ready_tasks().await succeeds, add an info-level log that clearly states ready-task recovery/resend completed so scheduler restarts can be confirmed in logs. Use the existing logging pattern around register and the runtime::run flow to keep the message consistent.components/spider-proto-rust/src/unpack/storage.rs (1)
209-225: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnbounded
wait_mscould allow arbitrarily long blocking polls.
max_itemsis checked to fitusize, butwait_ms/Duration::from_millis(self.wait_ms)has no upper bound. A client can request an effectively unbounded wait (e.g.,u64::MAXms), tying up a server-side poll for the whole duration unless it's independently capped downstream inServiceState::poll_ready_tasks. Consider clampingwait_ms(and possiblymax_items) to a sane maximum here so misbehaving/malicious clients can't hold connections open indefinitely.💡 Illustrative fix
+const MAX_POLL_WAIT: Duration = Duration::from_secs(60); + impl RequestUnpack for PollReadyTasksRequest { type Unpacked = (usize, Duration); fn unpack(self) -> Result<Self::Unpacked, UnpackError> { let max_items = usize::try_from(self.max_items).map_err(|_| { invalid_argument(format!( "max_items does not fit in `usize`: {}", self.max_items )) })?; - Ok((max_items, Duration::from_millis(self.wait_ms))) + let wait = Duration::from_millis(self.wait_ms).min(MAX_POLL_WAIT); + Ok((max_items, wait)) } }Please confirm whether
ServiceState::poll_ready_tasksalready bounds the wait time internally (e.g., against a shutdown/cancellation signal); if so this can be deprioritized.🤖 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-rust/src/unpack/storage.rs` around lines 209 - 225, In RequestUnpack for PollReadyTasksRequest, wait_ms is converted directly into Duration::from_millis without any upper bound, so clamp or reject overly large values alongside the existing max_items usize check. Update the unpack logic to enforce a sane maximum for wait_ms before returning the tuple, and verify whether ServiceState::poll_ready_tasks already caps blocking time; if not, add the limit here so PollReadyTasksRequest cannot request an unbounded poll.
🤖 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-proto-rust/src/unpack/storage.rs`:
- Around line 209-225: In RequestUnpack for PollReadyTasksRequest, wait_ms is
converted directly into Duration::from_millis without any upper bound, so clamp
or reject overly large values alongside the existing max_items usize check.
Update the unpack logic to enforce a sane maximum for wait_ms before returning
the tuple, and verify whether ServiceState::poll_ready_tasks already caps
blocking time; if not, add the limit here so PollReadyTasksRequest cannot
request an unbounded poll.
In `@components/spider-scheduler/src/runtime.rs`:
- Around line 128-129: The resend_ready_tasks recovery path in runtime::run is
currently silent on success, unlike the nearby register success log. After
storage_client.resend_ready_tasks().await succeeds, add an info-level log that
clearly states ready-task recovery/resend completed so scheduler restarts can be
confirmed in logs. Use the existing logging pattern around register and the
runtime::run flow to keep the message consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1796357e-c9e7-4a08-a7f7-646d466cd54a
⛔ Files ignored due to path filters (1)
components/spider-proto-rust/src/generated/storage.rsis excluded by!**/generated/**
📒 Files selected for processing (11)
components/spider-proto-rust/src/lib.rscomponents/spider-proto-rust/src/scheduler_registration.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/runtime.rscomponents/spider-scheduler/src/storage_client/grpc.rscomponents/spider-scheduler/src/storage_client/mod.rscomponents/spider-storage/src/grpc.rscomponents/spider-storage/src/state.rs
💤 Files with no reviewable changes (1)
- components/spider-scheduler/src/error.rs
LinZhihao-723
requested changes
Jul 5, 2026
LinZhihao-723
left a comment
LinZhihao-723
left a comment
Member
There was a problem hiding this comment.
Did you miss what we've discussed offline? This implementation is fundamentally wrong: resend_ready_tasks shouldn't be a gRPC call. Instead, it should be executed internally as a part of scheduler registration, not explicitly called by the scheduler.
Member
|
Close this PR for the reason pointed in #372 (review). |
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.
Note
This PR depends on #364.
Description
This PR resolves #352 by:
ResendReadyTasksgRPC protocol.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes