feat(spider-execution-manager): Add the execution-manager binary.#362
Merged
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds shared YAML config helpers, migrates storage off its local loader, introduces execution-manager config types, refactors liveness/runtime client ownership, and adds a new execution-manager CLI binary. ChangesShared config utilities and storage migration
Execution manager config, runtime refactor, and binary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
LinZhihao-723
changed the title
![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: 3
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/config.rs (1)
24-26: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winKeep the old config-loading surface or update the binary call site
components/spider-storage/src/bin/grpc_server.rsstill callsServerConfig::from_yaml_file(...), so removing that API here breaks the existing binary. A small forwarding shim/re-export would preserve compatibility for now.🤖 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/config.rs` around lines 24 - 26, The config-loading API removal breaks the existing grpc server call site, so restore compatibility by keeping a forwarding shim or re-export on ServerConfig. Update or add the config-loading method in DatabaseConfig/ServerConfig so grpc_server.rs can still call ServerConfig::from_yaml_file(...) without changes, or adjust that binary to use the new loading surface consistently.
🤖 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/bin/execution_manager.rs`:
- Around line 65-72: The ctrl_c handling in execution_manager::main treats
listener setup failures like a normal shutdown, which is incorrect. Update the
tokio::select! branch for tokio::signal::ctrl_c() so that an Err from ctrl_c()
is returned immediately as a startup failure after logging, and only log
“Received Ctrl-C” plus call cancellation_token.cancel() when the signal listener
succeeds with Ok(()). Keep the fix localized to the ctrl_c branch and the
surrounding run_handle.await flow.
In `@components/spider-execution-manager/src/config.rs`:
- Around line 56-61: Reject zero heartbeat intervals in LivenessConfig by
changing the storage_heartbeat_interval_sec and scheduler_heartbeat_interval_sec
fields to a non-zero type such as NonZeroU64 so invalid YAML cannot deserialize
to 0. Update RuntimeConfig construction to read these values with .get() before
creating Durations, and keep the fix localized around LivenessConfig and
RuntimeConfig.
In `@components/spider-utils/src/config.rs`:
- Around line 50-79: `EndpointConfig` currently uses `IpAddr`, which is fine for
binding but too restrictive for dialing gRPC targets that may be hostnames.
Update the config so the client-facing endpoint construction in
`EndpointConfig::endpoint` can accept DNS names/authority values instead of only
IPs, or split the bind and dial settings into separate types. Keep `socket_addr`
only for bind use cases, and adjust deserialization/validation around `host` so
`scheduler.default.svc.cluster.local`-style targets can be represented.
---
Outside diff comments:
In `@components/spider-storage/src/config.rs`:
- Around line 24-26: The config-loading API removal breaks the existing grpc
server call site, so restore compatibility by keeping a forwarding shim or
re-export on ServerConfig. Update or add the config-loading method in
DatabaseConfig/ServerConfig so grpc_server.rs can still call
ServerConfig::from_yaml_file(...) without changes, or adjust that binary to use
the new loading surface consistently.
🪄 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: 659c0830-f9d9-43e3-8b42-6aacb2c3bee5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
components/spider-execution-manager/Cargo.tomlcomponents/spider-execution-manager/src/bin/execution_manager.rscomponents/spider-execution-manager/src/client/liveness.rscomponents/spider-execution-manager/src/config.rscomponents/spider-execution-manager/src/lib.rscomponents/spider-execution-manager/src/liveness.rscomponents/spider-execution-manager/src/runtime.rscomponents/spider-storage/Cargo.tomlcomponents/spider-storage/src/bin/grpc_server.rscomponents/spider-storage/src/config.rscomponents/spider-storage/src/lib.rscomponents/spider-utils/Cargo.tomlcomponents/spider-utils/src/config.rscomponents/spider-utils/src/lib.rs
💤 Files with no reviewable changes (1)
- components/spider-storage/Cargo.toml
sitaowang1998
approved these changes
Jun 28, 2026
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.
Description
Summary
This PR adds the
spider_execution_managerbinary that boots the execution manager: it loads a YAML config, connects the storage, liveness, and scheduler gRPC clients, builds theRuntime, and runs it until Ctrl-C. Along the way it factors the config-loading boilerplate shared withspider-storageintospider-utils(anEndpointConfig, aConfigError, and aYamlConfigtrait), and drops theArcthat the liveness public API previously forced on callers.New binary (
spider-execution-manager/src/bin/execution_manager.rs)spider-storage'sgrpc_serverentrypoint: installs the shared non-blocking JSON logger viaspider_utils::logging::set_up_logging, then parses a single--config <PATH>argument withclap.Configfrom the YAML file, builds the storage and schedulertonic::Endpoints from the config, and connects all three gRPC clients (GrpcStorageClient,GrpcLivenessClient,GrpcSchedulerClient) through the connection pool with the configuredconnection_pool_size. The liveness client reuses the storage endpoint, since storage hosts the liveness service.Runtimefrom the clients and the derivedRuntimeConfig, logs the assigned execution-manager id, then spawnsruntime.run()and races it againsttokio::signal::ctrl_c()in atokio::select!. On Ctrl-C it cancels the runtime's token and awaits a graceful shutdown; if the runtime exits on its own first, its result is propagated. Each fallible boot step logs a contextual error before returning.inspect_errso a failed boot surfaces a specific message (config load, endpoint parse, per-client connect, runtime create) rather than a bare error.Execution-manager config (
spider-execution-manager/src/config.rs)Config, deserialized from YAML, holding the advertisedhost, the storage and schedulerEndpointConfigs, the liveness and task-executor sub-configs, theconnection_pool_size, andscheduler_poll_wait_ms.Config::runtime_config()derives the runtime'sRuntimeConfig(mapping the liveness heartbeat intervals toDurations and forwarding the task-executor paths).from_yaml_fileis provided by the sharedYamlConfigtrait rather than an inherent method.Shared config utilities (
spider-utils/src/config.rs)EndpointConfig { host, port }withsocket_addr()andendpoint(), the latter building a plaintext-HTTP/2tonic::Endpoint(IPv6-safe viaSocketAddr).ConfigError(Io/Parse) and aYamlConfigtrait whose defaultfrom_yaml_fileopens the file and deserializes it withyaml_serde. A blanketimpl<ConfigType: DeserializeOwned> YamlConfig for ConfigType {}gives every deserializable config type the loader for free; callers just bring the trait into scope.spider-storage'sServerConfigdrops its inherentfrom_yaml_fileand localConfigErrorand adopts the shared trait, so the two services no longer duplicate the loader.Drop the
Arcfrom the liveness public APIliveness::spawnandRuntime::createnow take the liveness client by value (LivenessClient + Clone + 'static) instead ofArc<LivenessClientType>. The realGrpcLivenessClientis already a cheap,Arc-backed clone (it wraps aConnectionPool), so the outerArcwas redundant; the binary now passes the client directly.impl<LivenessClientType: LivenessClient + ?Sized> LivenessClient for Arc<LivenessClientType>that forwards through the shared pointer, so callers who genuinely want shared ownership — such as the actor unit tests that retain a handle to inspect the mock — keep working with no changes.Arcis now opt-in rather than mandated.Notes
Configrequireshostto be set explicitly; an earlier draft auto-detected the local IP routed toward storage, but that was dropped in favor of taking the advertised IP from config.Checklist
breaking change.
Validation performed
Warning
The binary is not tested with storage/scheduler endpoints yet since the required services are not fully implemented.
Summary by CodeRabbit