feat: change gateway code as RFC-0003 #219
Conversation
WalkthroughThe recent updates focus on streamlining and renaming key components within the system. The "Inner-Gateway" module has been renamed to "Gateway" and now features an updated address format. Similarly, "WebRTC" and "RTMP" modules are now referred to as nodes. New constants have been introduced, enhancing the service architecture. Additionally, scripts and server functionalities have been adjusted to align with these changes, including modifications to parameters and the handling of zones instead of groups, reflecting a more intuitive and simplified structure. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
Cargo.lockis excluded by:!**/*.lockservers/media-server/Cargo.tomlis excluded by:!**/*.toml
Files selected for processing (20)
- README.md (1 hunks)
- packages/cluster/src/define/mod.rs (1 hunks)
- packages/cluster/src/define/rpc/gateway.rs (2 hunks)
- packages/cluster/src/implement/mod.rs (4 hunks)
- packages/cluster/src/implement/server.rs (2 hunks)
- servers/media-server/scripts/gateway.sh (1 hunks)
- servers/media-server/scripts/gateway_other.sh (1 hunks)
- servers/media-server/scripts/media_rtmp.sh (1 hunks)
- servers/media-server/scripts/media_sip.sh (1 hunks)
- servers/media-server/scripts/media_webrtc.sh (1 hunks)
- servers/media-server/src/main.rs (4 hunks)
- servers/media-server/src/server/gateway.rs (12 hunks)
- servers/media-server/src/server/gateway/logic.rs (8 hunks)
- servers/media-server/src/server/gateway/logic/global_registry.rs (9 hunks)
- servers/media-server/src/server/gateway/logic/inner_registry.rs (5 hunks)
- servers/media-server/src/server/gateway/rpc.rs (1 hunks)
- servers/media-server/src/server/gateway/rpc/http.rs (1 hunks)
- servers/media-server/src/server/gateway/webrtc_route.rs (4 hunks)
- servers/media-server/src/server/rtmp.rs (5 hunks)
- servers/media-server/src/server/webrtc.rs (5 hunks)
Additional comments: 54
servers/media-server/scripts/media_rtmp.sh (1)
- 5-5: Change from
--sdn-groupto--sdn-zonecorrectly implements the specified objective.servers/media-server/scripts/media_webrtc.sh (1)
- 5-5: Change from
--sdn-groupto--sdn-zonecorrectly implements the specified objective.servers/media-server/scripts/media_sip.sh (1)
- 5-5: Change from
--sdn-groupto--sdn-zonecorrectly implements the specified objective.servers/media-server/scripts/gateway.sh (1)
- 5-5: Addition of
--sdn-zonecorrectly implements the specified objective.servers/media-server/scripts/gateway_other.sh (1)
- 5-5: Addition of
--sdn-zonewith a different zone value correctly demonstrates the system's flexibility and scalability.servers/media-server/src/server/gateway/rpc.rs (1)
- 9-9: Renaming
BestNodesttoBestNodescorrects a typo and improves code clarity.packages/cluster/src/define/mod.rs (1)
- 64-64: Consolidation and addition of service constants correctly implement the specified objectives and enhance system modularity.
packages/cluster/src/define/rpc/gateway.rs (1)
- 23-23: Using
zoneinstead ofgroupinNodePingand addingservice_idinQueryBestNodesResponsecorrectly implement the specified objectives.packages/cluster/src/implement/server.rs (2)
- 52-52: Returning an additional
PubsubSdkinstance from thenewfunction ofServerSdncorrectly implements the specified objectives.- 106-106: Cloning the
pubsub_sdkfield in the struct initialization correctly implements the specified objectives.servers/media-server/src/server/gateway/webrtc_route.rs (2)
- 22-22: Adding
RouteResultto the import list correctly implements the specified objectives.- 111-111: Modifying the
route_to_nodefunction signature to remove thedest_service_idparameter and handle the service ID within the function correctly implements the specified objectives.servers/media-server/src/main.rs (3)
- 11-11: Renaming services to a unified
GATEWAY_SERVICEcorrectly implements the specified objectives.- 45-47: Renaming the
sdn_groupparameter tosdn_zonecorrectly implements the specified objectives.- 106-107: Simplifying the logic for setting
local_tagsandconnect_tagscorrectly implements the specified objectives.servers/media-server/src/server/rtmp.rs (3)
- 15-15: Renaming
INNER_GATEWAY_SERVICEtoGATEWAY_SERVICEcorrectly implements the specified objectives.- 67-67: Adding a new parameter
zoneto therun_rtmp_serverfunction correctly implements the specified objectives.- 192-195: Adding a new parameter
zoneto theping_gatewayfunction correctly implements the specified objectives.README.md (2)
- 130-130: Renaming the "Inner-Gateway" module to the "Gateway" node in the documentation correctly implements the specified objectives.
- 137-137: Renaming the "WebRTC" and "RTMP" modules to nodes in the documentation correctly implements the specified objectives.
servers/media-server/src/server/gateway/logic.rs (8)
- 14-19: The
RouteResultenum is correctly defined and aligns with the described changes.- 23-24: The method signatures for
on_pingandbest_nodesin theServiceRegistrytrait have been updated to include new parameters and return types. Ensure all implementations of this trait are updated accordingly.Verification successful
The review comment mentioned updates to the method signatures for
on_pingandbest_nodesin theServiceRegistrytrait, specifically including new parameters and return types. The verification process involved checking the implementations of these methods inServiceInnerRegistryandServiceGlobalRegistry.From the provided script outputs, it's clear that both
ServiceInnerRegistryandServiceGlobalRegistryhave implemented theon_pingandbest_nodesmethods with the updated signatures as described in the review comment. Theon_pingmethod includes parameters such asnow_ms,zone,location,node_id,usage,live, andmax. Thebest_nodesmethod includes parameters such aslocation,max_usage,max_usage_fallback, andsize, and returns aRouteResult.Given this information, it can be concluded that all implementations of the
ServiceRegistrytrait have been updated accordingly with the new parameters and return types for bothon_pingandbest_nodesmethods.* 44-46: The split of service registries into `global_gateways` and `inner_services` based on service types is a logical reorganization. Ensure that the logic handling these registries is correctly updated throughout the file.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify all implementations of ServiceRegistry have updated method signatures. rg "impl ServiceRegistry" -A 10Length of output: 2641
Script:
#!/bin/bash # View the implementations of on_ping and best_nodes methods in ServiceInnerRegistry rg "fn on_ping\(&mut self" servers/media-server/src/server/gateway/logic/inner_registry.rs -A 10 rg "fn best_nodes\(&mut self" servers/media-server/src/server/gateway/logic/inner_registry.rs -A 10 # View the implementations of on_ping and best_nodes methods in ServiceGlobalRegistry rg "fn on_ping\(&mut self" servers/media-server/src/server/gateway/logic/global_registry.rs -A 10 rg "fn best_nodes\(&mut self" servers/media-server/src/server/gateway/logic/global_registry.rs -A 10Length of output: 2326
Verification successful
The examination of the
logic.rsfile reveals that bothglobal_gatewaysandinner_servicesare actively utilized and managed throughout the file. This includes their initialization, iteration, modification, and assertions in test scenarios, indicating a comprehensive and correct update of the logic handling these registries as per the split into two separate HashMaps based on service types. Therefore, the changes mentioned in the review comment have been successfully verified.* 80-88: The `on_node_ping` method correctly handles pings for different service types by delegating to `on_node_ping_service`. Ensure that the logic within `on_node_ping_service` correctly updates the respective service registry based on the service type.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for correct handling of global_gateways and inner_services throughout the file. rg "global_gateways|inner_services"Length of output: 1657
Verification successful
The
on_node_ping_servicemethod's implementation confirms that it correctly updates the service registry based on the service type, provided the ping is from the correct zone. This aligns with the expectations set in the review comment, verifying that the method functions as intended for handling pings for different service types. Therefore, the logic withinon_node_ping_serviceis implemented as expected to update the respective service registry based on the service type.* 100-110: The `on_gateway_ping` method's implementation mirrors that of `on_node_ping`, appropriately handling pings for gateway services. This consistency in handling pings is good practice. * 126-139: The `best_nodes` method's logic to first check global gateways and then inner services for routing decisions is sound. The use of `RouteResult` enum for the return type enhances readability and maintainability. * 151-158: The `on_node_ping_service` and `on_gateway_ping_service` methods correctly differentiate handling based on the zone. This zone-based logic is crucial for the intended functionality. Ensure that zone comparisons are case-sensitive or not based on the application's requirements. * 201-207: The tests have been updated to reflect the changes in the gateway logic. Ensure that all new logic paths introduced are covered by these tests.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure on_node_ping_service updates the correct service registry. rg "on_node_ping_service" -A 10Length of output: 2813
servers/media-server/src/server/gateway/logic/inner_registry.rs (3)
- 82-82: The renaming of
_groupto_zonein theon_pingfunction parameters aligns with the shift towards zone-based logic. Ensure consistency in the use of thezoneparameter across all related functions.Verification successful
The review comment regarding the renaming of
_groupto_zonein theon_pingfunction and ensuring consistency in the use of thezoneparameter across all related functions is supported by the evidence from the codebase. The usage of_zonein various parts of the application, including server configuration and logic, indicates a consistent shift towards zone-based logic. Therefore, the changes align with the broader application context.* 101-132: The refactoring of the `best_nodes` function to use `RouteResult` enum improves the clarity of return types. Ensure that all callers of `best_nodes` correctly handle the new enum-based return values.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent use of the `zone` parameter across related functions. rg "_zone" -A 3 -B 3Length of output: 14621
Verification successful
The review of the
best_nodesfunction and its callers indicates that the new enum-based return values are being correctly handled across the codebase. The explicit checks forRouteResultenum variants in both the application logic and tests confirm that the refactoring has been successfully integrated. Based on the provided script output, there is no evidence of issues with how the new return types are managed by the callers ofbest_nodes.* 271-277: The assertion modifications in tests to accommodate the updated `RouteResult` structure are appropriate. Verify that all relevant test cases have been updated to reflect these changes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure callers of best_nodes handle the new enum-based return values. rg "best_nodes" -A 5 -B 5Length of output: 19342
packages/cluster/src/implement/mod.rs (1)
- 29-29: The addition of the
_pubsubparameter in theServerSdn::newfunction call is consistent across test functions. Ensure that the_pubsubparameter is correctly utilized or documented if it's intended for future use.servers/media-server/src/server/gateway/logic/global_registry.rs (6)
- 27-27: The addition of the
zonefield to theZonestruct aligns with the shift towards zone-based logic. Ensure that thezonefield is correctly utilized in all relevant logic.Verification successful
The examination of the
zonefield usage across various parts of the system confirms its broad and consistent integration. This includes server configuration, gateway logic, and node pinging processes, aligning with the initial review comment about ensuring the correct utilization of thezonefield in all relevant logic. Therefore, the changes related to the addition of thezonefield and its utilization are verified to be correctly implemented.* 38-38: The addition of the `zone` field to the `ServiceGlobalRegistry` struct is appropriate for supporting zone-based operations. Verify that this field is consistently used where zone differentiation is required.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for correct utilization of the `zone` field in all relevant logic. rg "zone:" -A 3 -B 3Length of output: 15153
Verification successful
The script output confirms the addition of the
zonefield to theServiceGlobalRegistrystruct and its active use in initializing the struct and within theon_pingmethod for zone-specific logic. This indicates that thezonefield is indeed being utilized where zone differentiation is required, aligning with the intent of supporting zone-based operations as mentioned in the review comment. Based on the available evidence, it appears that thezonefield is consistently used in the parts of the codebase examined.* 45-52: The modification of the `new` method in `ServiceGlobalRegistry` to accept a `zone` parameter is correctly implemented. Ensure that all instances of `ServiceGlobalRegistry::new` are updated to pass the `zone` parameter.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify consistent use of the `zone` field in ServiceGlobalRegistry operations. rg "zone:" -A 3 -B 3 servers/media-server/src/server/gateway/logic/global_registry.rsLength of output: 1571
Verification successful
The instances of
ServiceGlobalRegistry::newfound in the codebase correctly pass thezoneparameter as required. This confirms that the modification to include azoneparameter in thenewmethod has been properly implemented and adopted across various parts of the code, aligning with the initial review comment.* 93-96: The update to the `on_ping` method to use the `zone` parameter is correctly implemented. This change is crucial for supporting zone-based logic in service registries. * 126-140: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [119-137]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure all instances of ServiceGlobalRegistry::new pass the `zone` parameter. rg "ServiceGlobalRegistry::new" -A 3Length of output: 3237
The modification of the
best_nodesmethod to return aRouteResultand the logic to determine the closest zone or fallback zone is sound. This enhances the flexibility and accuracy of node selection based on zones.
- 154-156: The tests have been updated to reflect changes in the
ServiceGlobalRegistrymethods. Ensure that all new logic paths introduced are covered by these tests.servers/media-server/src/server/gateway.rs (11)
- 73-78: The addition of
zone: &strandpubsub: PubsubSdkparameters torun_gateway_serverfunction aligns with the summary. Ensure all calls to this function are updated accordingly.Verification successful
The search for calls to
run_gateway_serverwith a broader pattern successfully identified a call in./servers/media-server/src/main.rs:112. This call includes the parametersargs.http_port,args.http_tls,&args.sdn_zone,opts,ctx,cluster,rpc_endpoint, andpubsub, which aligns with the updated function signature that includeszone: &strandpubsub: PubsubSdkas mentioned in the review comment. Therefore, it can be concluded that the call torun_gateway_serverhas been updated accordingly to include the new parameters.* 123-123: The instantiation of `GatewayLogic` with the `zone` parameter is correct and aligns with the summary. * 126-129: The logic for handling pubsub channels and messages is correctly implemented as per the summary. * 137-147: The updated logic for handling node pings through pubsub channels is correct. Ensure serialization and deserialization of `ping` are compatible with expected data structures. * 152-162: The updated logic for handling gateway pings through pubsub channels is correct. Similar to node pings, ensure serialization compatibility. * 165-165: The renaming of `ping_global_gateway` to `ping_other_gateways` and its invocation is correctly implemented as per the summary. * 179-179: The logic for broadcasting a node ping to other nodes in the same zone is correctly implemented. * 194-208: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [184-201]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if all calls to run_gateway_server have been updated to include the new parameters. ast-grep --lang rust --pattern $'run_gateway_server($_, $_, $_, $_, $_, $_, $_, $_, $_)'Length of output: 89
Script:
#!/bin/bash # Broaden the search to find any calls to run_gateway_server, regardless of the number of parameters. ast-grep --lang rust --pattern $'run_gateway_server($$$)'Length of output: 241
The logic for handling
BestNodesrequests, including the use ofip2locationfor determining the requester's location and routing decisions, is correctly updated.
- 216-226: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [205-223]
The handling of
WhipConnectandWhepConnectrequests, including routing to the appropriate node based on location, is correctly updated.
- 241-241: The handling of
WebrtcConnectrequests, including routing to the appropriate node based on location, is correctly updated.- 300-311: The
ping_other_gatewaysfunction correctly implements the logic for sending pings to other gateways, including the newzoneparameter.servers/media-server/src/server/gateway/rpc/http.rs (1)
- 62-62: Corrected the method call from
BestNodesttoBestNodes, aligning with the summary.servers/media-server/src/server/webrtc.rs (4)
- 15-15: Renaming
INNER_GATEWAY_SERVICEtoGATEWAY_SERVICEis correctly implemented as per the summary.- 71-71: The addition of the
zone: &strparameter torun_webrtc_serverfunction is correct. Ensure all calls to this function are updated accordingly.Verification successful
Given the lack of output from the initial script, it's necessary to adjust our approach to ensure we're accurately capturing calls to
run_webrtc_serverwith the correct number of parameters, including the newly addedzoneparameter. Let's broaden the search to capture any calls to this function, regardless of the number of parameters, to verify if they've been updated accordingly.
The verification process has identified a call to
run_webrtc_serverin./servers/media-server/src/main.rs:125that includes the newzoneparameter (&args.sdn_zone), indicating that the function's calls have been updated accordingly. This aligns with the requirement mentioned in the review comment. Based on the available evidence, it appears that the addition of thezone: &strparameter to therun_webrtc_serverfunction and the update of its calls have been correctly implemented, at least for the instance found.* 126-126: The `ping_gateway` function correctly includes the new `zone` parameter, aligning with the summary. * 391-394: The `ping_gateway` function's implementation, including the `zone` parameter, is correct and aligns with the summary.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if all calls to run_webrtc_server have been updated to include the new `zone` parameter. ast-grep --lang rust --pattern $'run_webrtc_server($_, $_, $_, $_, $_, $_)'Length of output: 76
Script:
#!/bin/bash # Broaden the search to capture any calls to run_webrtc_server, to verify if the new `zone` parameter is included. ast-grep --lang rust --pattern $'run_webrtc_server($$$)'Length of output: 231
| @@ -3,23 +3,24 @@ use std::{sync::Arc, time::Duration}; | |||
| use async_std::stream::StreamExt; | |||
| use clap::Parser; | |||
| use cluster::{ | |||
| atm0s_sdn::{Publisher, PubsubSdk}, | |||
There was a problem hiding this comment.
Missing removal of NodePong from rpc::gateway imports as mentioned in the summary.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #219 +/- ##
==========================================
- Coverage 50.32% 50.25% -0.08%
==========================================
Files 155 155
Lines 13851 13934 +83
==========================================
+ Hits 6971 7002 +31
- Misses 6880 6932 +52 ☔ View full report in Codecov by Sentry. |
This PR implement gateway as described in RFC-0003
Changes: now we have only one type gateway node, which works for both global and inner