Rework relay control plane interface #99
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the relay implementation to use a generic ControlPlane trait abstraction, replacing the previously hardcoded Api client. This enables supporting different control plane implementations beyond just HTTP-based routing.
- Introduced a
ControlPlanetrait with generic implementations for cross-relay routing - Replaced direct
Apiusage with the generic control plane pattern throughout the codebase - Created
HttpControlPlaneas the concrete HTTP-based implementation (equivalent to the oldApi)
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| moq-relay-ietf/src/control_plane.rs | Defines the new ControlPlane and ControlPlaneRefresher traits for control plane abstraction |
| moq-relay-ietf/src/control_plane_http.rs | Implements HTTP-based control plane using the moq-api client |
| moq-relay-ietf/src/lib.rs | Creates library structure with RelayServer wrapper and public exports |
| moq-relay-ietf/src/api.rs | Removed - functionality migrated to control_plane_http.rs |
| moq-relay-ietf/src/session.rs | Added generic ControlPlane parameter to Session struct |
| moq-relay-ietf/src/remote.rs | Added generic ControlPlane parameter to remote-related structs |
| moq-relay-ietf/src/relay.rs | Updated to use generic control plane and split node_url handling |
| moq-relay-ietf/src/producer.rs | Added generic ControlPlane parameter to Producer |
| moq-relay-ietf/src/consumer.rs | Updated to use control plane trait methods and added node_url field |
| moq-relay-ietf/src/main.rs | Simplified to use new RelayServer API with HttpControlPlane |
| moq-relay-ietf/Cargo.toml | Added library/binary configuration and async-trait dependency |
| Cargo.lock | Updated with async-trait dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This is a stub implementation - in practice you'd need valid URLs | ||
| // The actual instance should be created via `new()` with proper config | ||
| panic!("HttpControlPlane requires API URL and node URL - use HttpControlPlane::new() instead") |
There was a problem hiding this comment.
The Default implementation for HttpControlPlane panics at runtime, which is problematic since ControlPlane trait requires Default. This creates a trait implementation that violates the principle of least surprise and will cause runtime panics. Consider either removing the Default requirement from the ControlPlane trait, or providing a safe default implementation (e.g., with placeholder URLs or an Option-wrapped state).
| // This is a stub implementation - in practice you'd need valid URLs | |
| // The actual instance should be created via `new()` with proper config | |
| panic!("HttpControlPlane requires API URL and node URL - use HttpControlPlane::new() instead") | |
| // Safe default using placeholder URLs | |
| let api_url = Url::parse("http://localhost").unwrap(); | |
| let node_url = Url::parse("http://localhost").unwrap(); | |
| HttpControlPlane::new(api_url, node_url) |
| tokio::spawn(async move { | ||
| web.run().await.expect("failed to run web server"); | ||
| }); |
There was a problem hiding this comment.
The web server task is spawned without capturing its join handle, making it impossible to detect or propagate web server failures. If the web server panics or fails, the main relay will continue running without any indication of the failure. Consider either returning the join handle or logging the error instead of using expect.
| }; | ||
|
|
||
| relay.run().await | ||
| let server = RelayServer::new(config, control_plane, cli.node)?; |
There was a problem hiding this comment.
The cli.node is passed twice - once when creating control_plane (line 73-74) and again when creating the server. This duplication could lead to inconsistencies. Consider extracting the node URL from the control plane or restructuring to avoid passing it separately.
| let server = RelayServer::new(config, control_plane, cli.node)?; | |
| // Extract node URL from control_plane if present, otherwise use cli.node | |
| let node_url = match &control_plane { | |
| Some(cp) => cp.node_url().clone(), | |
| None => cli.node.clone().expect("node URL must be provided"), | |
| }; | |
| let server = RelayServer::new(config, control_plane, node_url)?; |
2 participants
Reworks the interface between
moq-relay-ietfand some kind of control plane to assist with relay to relay routing decisions.This is part of what's needed to allow us to depend directly on open source moq-rs instead of our current internal fork.