balancer: fix clients not being removed from balancer context on disconnect #1212
Conversation
WalkthroughThe recent update involves a modification to the load balancer's client management system, specifically introducing functionality to remove a client from the internal tracking map. This change suggests an enhancement in the way the balancer maintains and updates its list of active clients. Changes
Poem
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 selected for processing (1)
- crates/ott-balancer/src/balancer.rs (1 hunks)
Additional comments: 1
crates/ott-balancer/src/balancer.rs (1)
- 212-218: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-214]
While the specific change to
remove_clientis correct, it's important to review the entire flow of client disconnection to ensure that there are no other places where the client should be removed fromself.clientsbut isn't. This includes checking all the paths that lead to a client disconnection and ensuring thatremove_clientis called appropriately in each case.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1212 +/- ##
===========================================
Coverage 61.5318% 61.5318%
===========================================
Files 115 115
Lines 9322 9322
Branches 1141 1141
===========================================
Hits 5736 5736
Misses 3586 3586 ☔ View full report in Codecov by Sentry. |
Passing run #802 ↗︎
Details:
Review all test suite changes for PR #1212 ↗︎ |
|||||||||||||||
dyc3
changed the title
…onnect possibly related to #1207?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/ott-balancer/src/balancer.rs (2 hunks)
Additional comments: 2
crates/ott-balancer/src/balancer.rs (2)
- 215-215: The change to remove the client from the
self.clientsmap upon disconnection is crucial for maintaining the integrity of the balancer's state. It's good to see that this fix has been implemented. However, it's important to ensure that this change is covered by unit tests to verify its correctness.Verification successful
The provided script output confirms that the
test_clients_add_removefunction in thebalancer.rsfile includes logic to test the removal of a client from theBalancerContext. The test adds a client, asserts that the client is in the context, then removes the client and asserts that the client is no longer in the context. This indicates that the unit test covers the scenario where a client is removed, which aligns with the changes made in the pull request.* 708-781: The unit test `test_clients_add_remove` appears to cover the scenario of adding and removing a client. It's important to ensure that the test adequately tests the functionality and that it passes successfully.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the unit test `test_clients_add_remove` covers the scenario where a client is removed. rg --type rust "test_clients_add_remove" crates/ott-balancer/src/balancer.rsLength of output: 117
Script:
#!/bin/bash # Extract the contents of the test function `test_clients_add_remove` to verify if it covers the client removal scenario. rg --type rust --context 10 "async fn test_clients_add_remove" crates/ott-balancer/src/balancer.rsLength of output: 881
Script:
#!/bin/bash # Request the continuation of the test function `test_clients_add_remove` to see the test logic for client removal. rg --type rust --after-context 40 "async fn test_clients_add_remove" crates/ott-balancer/src/balancer.rsLength of output: 1723
Script:
#!/bin/bash # Request the continuation of the test function `test_clients_add_remove` beyond the current output to see the test logic for client removal. rg --type rust --after-context 60 "async fn test_clients_add_remove" crates/ott-balancer/src/balancer.rsLength of output: 2302
| async fn test_clients_add_remove() { | ||
| // a bunch of setup | ||
| let room_name = RoomName::from("test"); | ||
| let mut ctx = BalancerContext::new(); |
There was a problem hiding this comment.
BalancerContext is simply a struct that describes the state of the balancer, including quantity of rooms per monolith, how many instances of a monolith per region, etc. correct?
| }, | ||
| proxy_port: 3000, | ||
| }, | ||
| monolith_outbound_tx, |
There was a problem hiding this comment.
Why are these defined in a different scope to that of the other values? Is it because the new() constructor takes them as arguments?
There was a problem hiding this comment.
The channels need to be defined at the function level context so they stay alive for the entire unit test. Otherwise the channels get disposed immediately and the test will fail.
Summary by CodeRabbit