refactor(spider-huntsman): Use auto-incrementing u64 IDs instead of UUIDv7 for database-generated IDs. - #337
Conversation
WalkthroughThis PR migrates the core Id from UUID-backed to u64-backed representation, updates serde/Display/SQLx impls, converts MariaDB DDL and queries to BIGINT auto-increment IDs, and updates production logging and test fixtures to use numeric/random ID constructors. ChangesUUID-to-u64 ID Type Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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-storage/src/db/mariadb.rs (1)
599-615:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd a real migration path for existing MariaDB schemas.
These new definitions only affect fresh databases. The connector still bootstraps with
CREATE TABLE IF NOT EXISTS, so any existing deployment keeps its old UUID/BINARY(16) columns while the rest of this PR now binds and decodes the same IDs asu64. That turns an in-place upgrade into a runtime schema mismatch instead of a clean migration. Please add an explicitALTER TABLEpath or a fail-fast schema-version check before serving traffic.Also applies to: 640-646
🧹 Nitpick comments (1)
components/spider-core/src/types/id.rs (1)
35-42: 🏗️ Heavy liftFence
Id::random()behind a test-only API surface.The doc comment already says production IDs must come from persistent storage, but this helper is still callable in every build. Consider moving it behind a
test-utilsfeature (or a dedicated test helper crate) so downstream tests keep it without inviting production code to mint synthetic DB-owned IDs.🤖 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-core/src/types/id.rs` around lines 35 - 42, The Id::random() helper is intended for tests only, so restrict its availability by gating it behind a test-only feature: annotate the function with #[cfg(any(test, feature = "test-utils"))] (or just feature="test-utils" if you prefer) and add #[cfg_attr(docsrs, doc(cfg(feature = "test-utils")))] to keep docs clear; this preserves Id::random() for downstream tests when the feature is enabled but prevents production code from calling the minting helper. Ensure the function signature remains pub fn random() -> Self but only compiled when the feature (or test) is enabled.
🤖 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-core/src/types/id.rs`:
- Around line 35-42: The Id::random() helper is intended for tests only, so
restrict its availability by gating it behind a test-only feature: annotate the
function with #[cfg(any(test, feature = "test-utils"))] (or just
feature="test-utils" if you prefer) and add #[cfg_attr(docsrs, doc(cfg(feature =
"test-utils")))] to keep docs clear; this preserves Id::random() for downstream
tests when the feature is enabled but prevents production code from calling the
minting helper. Ensure the function signature remains pub fn random() -> Self
but only compiled when the feature (or test) is enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e24c4af8-2864-455e-bb83-f0dc322c24d7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
components/spider-core/Cargo.tomlcomponents/spider-core/src/types/id.rscomponents/spider-execution-manager/src/liveness.rscomponents/spider-execution-manager/src/process_pool.rscomponents/spider-storage/Cargo.tomlcomponents/spider-storage/src/db/mariadb.rscomponents/spider-storage/src/state/job_cache.rscomponents/spider-storage/src/task_instance_pool.rscomponents/spider-storage/tests/mariadb_infra.rscomponents/spider-storage/tests/mariadb_test.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/tests/test_process_pool.rstests/huntsman/tdl-integration/tests/complex.rstests/huntsman/test-utils/src/executor.rstests/huntsman/test-utils/src/mock.rs
💤 Files with no reviewable changes (1)
- components/spider-storage/Cargo.toml
LinZhihao-723
left a comment
There was a problem hiding this comment.
Polished the ID implementation a bit. Otherwise lgtm. For the PR title, how about:
refactor(spider-huntsman): Use auto-incrementing u64 IDs instead of UUIDv7 for database-generated IDs.
Uuid to u64.
Description
This PR changes the following ids from
Uuidtou64:Note that the task id stays
usizeas it is used as an index to vectors.This PR also changes the database schema to use
AUTOINCREMENT BIGINT UNSIGNEDto match the change.The old id
defaultandnewfunctions, both only used for tests, has changed to:defaultalways return 0newrenames torandomand returns a randomly generatedu64This fulfills the need for tests. In production, neither functions should be used, as ids are generated by database.
Checklist
breaking change.
Validation performed
Uuidleft, in code and in dependency.Summary by CodeRabbit
Chores
Tests