refactor(huntsman): Unify TaskId by replacing spider-core's definition with spider-storage's.#331
Open
LinZhihao-723 wants to merge 1 commit into
Open
refactor(huntsman): Unify TaskId by replacing spider-core's definition with spider-storage's.#331LinZhihao-723 wants to merge 1 commit into
TaskId by replacing spider-core's definition with spider-storage's.#331LinZhihao-723 wants to merge 1 commit into
Conversation
Contributor
WalkthroughThis pull request migrates the ChangesTaskId type migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
components/spider-tdl/src/task_context.rs (1)
30-40: ⚡ Quick winCover
CommitandCleanupin this round-trip test.
TaskContext.task_idnow has three wire shapes, but this test only exercisesTaskId::Index(0). A small table-driven round-trip overIndex,Commit, andCleanupwould keep the termination-task serialization path covered too.🧪 Suggested test shape
#[test] fn round_trip_msgpack() -> anyhow::Result<()> { - let ctx = TaskContext { - job_id: JobId::new(), - task_id: TaskId::Index(0), - task_instance_id: 13, - resource_group_id: ResourceGroupId::new(), - }; - let encoded = rmp_serde::to_vec(&ctx)?; - let decoded: TaskContext = rmp_serde::from_slice(&encoded)?; - assert_eq!(decoded, ctx); + for task_id in [TaskId::Index(0), TaskId::Commit, TaskId::Cleanup] { + let ctx = TaskContext { + job_id: JobId::new(), + task_id, + task_instance_id: 13, + resource_group_id: ResourceGroupId::new(), + }; + let encoded = rmp_serde::to_vec(&ctx)?; + let decoded: TaskContext = rmp_serde::from_slice(&encoded)?; + assert_eq!(decoded, ctx); + } 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-tdl/src/task_context.rs` around lines 30 - 40, Update the round_trip_msgpack test to iterate over the three TaskId wire shapes so Commit and Cleanup are covered: create a small table (or array) of TaskId variants including TaskId::Index(0), TaskId::Commit(some_id) and TaskId::Cleanup(some_id), and for each variant construct a TaskContext (using the existing fields like job_id, task_instance_id, resource_group_id), serialize with rmp_serde::to_vec and deserialize with rmp_serde::from_slice, and assert_eq!(decoded, original) for every case; modify the round_trip_msgpack function to loop over these TaskId variants so the termination-task serialization paths (Commit and Cleanup) are exercised.tests/huntsman/task-executor/src/lib.rs (1)
186-202: ⚡ Quick winUpdate docstring to reflect fixed
task_idvalue.The docstring states "The id fields are fresh per call", but
task_idis now fixed atTaskId::Index(0)whilejob_idandresource_group_idremain fresh. The docstring should clarify this distinction.📝 Proposed docstring update
/// # Returns /// -/// A placeholder msgpack-encoded [`TaskContext`] suitable for a one-shot test invocation. The id -/// fields are fresh per call but the executor doesn't inspect them. +/// A placeholder msgpack-encoded [`TaskContext`] suitable for a one-shot test invocation. The +/// `job_id` and `resource_group_id` fields are fresh per call; `task_id` is fixed at `Index(0)`. +/// The executor doesn't inspect these values. /// /// # Panics🤖 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 `@tests/huntsman/task-executor/src/lib.rs` around lines 186 - 202, Update the docstring for build_ctx to accurately reflect that not all id fields are fresh: explain that job_id and resource_group_id are newly generated per call (JobId::new(), ResourceGroupId::new()) while task_id is fixed to TaskId::Index(0) and task_instance_id is 1; mention that encoding may panic on failure as before and keep the must_use note.tests/huntsman/task-executor/tests/test_process_pool.rs (1)
66-90: ⚡ Quick winUpdate docstring to clarify which IDs are fresh.
The docstring states "fresh IDs", but
task_idis now fixed atTaskId::Index(0)whilejob_idandresource_group_idremain fresh. Consider updating the docstring for clarity.📝 Proposed docstring update
/// # Returns /// -/// A request with fresh IDs, a placeholder [`TimeoutPolicy`] (which the pool ignores — the caller -/// supplies `hard_timeout` directly to [`ProcessPool::execute`]), and the supplied `inputs`. +/// A request with fresh `job_id` and `resource_group_id`, fixed `task_id` at `Index(0)`, a +/// placeholder [`TimeoutPolicy`] (which the pool ignores — the caller supplies `hard_timeout` +/// directly to [`ProcessPool::execute`]), and the supplied `inputs`. fn make_request(task_func: &str, inputs: Vec<TaskInput>) -> ExecuteRequest {🤖 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 `@tests/huntsman/task-executor/tests/test_process_pool.rs` around lines 66 - 90, The docstring for make_request is misleading: it says "fresh IDs" but only job_id and resource_group_id are generated fresh while task_id is fixed to TaskId::Index(0); update the function's docstring to explicitly state which IDs are fresh (job_id and resource_group_id) and that task_id is intentionally fixed to TaskId::Index(0) so callers understand the behavior; reference the make_request function and the fields job_id, resource_group_id, and task_id in the updated comment.
🤖 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-tdl/src/task_context.rs`:
- Around line 30-40: Update the round_trip_msgpack test to iterate over the
three TaskId wire shapes so Commit and Cleanup are covered: create a small table
(or array) of TaskId variants including TaskId::Index(0),
TaskId::Commit(some_id) and TaskId::Cleanup(some_id), and for each variant
construct a TaskContext (using the existing fields like job_id,
task_instance_id, resource_group_id), serialize with rmp_serde::to_vec and
deserialize with rmp_serde::from_slice, and assert_eq!(decoded, original) for
every case; modify the round_trip_msgpack function to loop over these TaskId
variants so the termination-task serialization paths (Commit and Cleanup) are
exercised.
In `@tests/huntsman/task-executor/src/lib.rs`:
- Around line 186-202: Update the docstring for build_ctx to accurately reflect
that not all id fields are fresh: explain that job_id and resource_group_id are
newly generated per call (JobId::new(), ResourceGroupId::new()) while task_id is
fixed to TaskId::Index(0) and task_instance_id is 1; mention that encoding may
panic on failure as before and keep the must_use note.
In `@tests/huntsman/task-executor/tests/test_process_pool.rs`:
- Around line 66-90: The docstring for make_request is misleading: it says
"fresh IDs" but only job_id and resource_group_id are generated fresh while
task_id is fixed to TaskId::Index(0); update the function's docstring to
explicitly state which IDs are fresh (job_id and resource_group_id) and that
task_id is intentionally fixed to TaskId::Index(0) so callers understand the
behavior; reference the make_request function and the fields job_id,
resource_group_id, and task_id in the updated comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b475c7f-27cf-4706-8084-7473253a5015
📒 Files selected for processing (11)
components/spider-core/src/types/id.rscomponents/spider-storage/src/cache.rscomponents/spider-storage/src/cache/job.rscomponents/spider-storage/src/task_instance_pool.rscomponents/spider-storage/tests/scheduling_infra.rscomponents/spider-tdl/src/task.rscomponents/spider-tdl/src/task_context.rscomponents/spider-tdl/tests/test_task_macro.rstests/huntsman/task-executor/src/lib.rstests/huntsman/task-executor/tests/test_process_pool.rstests/huntsman/tdl-integration/tests/complex.rs
💤 Files with no reviewable changes (1)
- components/spider-storage/src/cache.rs
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
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.
Description
TaskIdwas defined in bothspider-coreandspider-storagewhile:spider-coreis a wrapper of UUID.spider-storageis an enum of {task index, commit, cleanup}.The definition in
spider-storageis the actual definition we use in the current implementation.Thus, we replace the definition in
spider-corewithspider-storage's one so that all components can use the same definition.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Refactor
Tests