balancer: unload duplicate rooms #1131
Merged
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## master #1131 +/- ##
===========================================
Coverage 60.4202% 60.4202%
===========================================
Files 115 115
Lines 9280 9280
Branches 1131 1131
===========================================
Hits 5607 5607
Misses 3673 3673 |
2 failed tests on run #678 ↗︎
Details:
|
|||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| An uncaught error was detected outside of a test |
Output
Screenshots
Video
|
|
tests/e2e/integration/playback.spec.ts • 1 failed test • E2E - electron
| Test | Artifacts | |
|---|---|---|
| Video playback > should add and play a video |
Output
Screenshots
Video
|
|
Review all test suite changes for PR #1131 ↗︎
cjrkoa
reviewed
Nov 6, 2023
|
|
||
| let recv = m2.collect_recv(); | ||
| assert_eq!(recv.len(), 1); | ||
| assert!(matches!(recv[0], MsgB2M::Unload(B2MUnload { .. }))); |
There was a problem hiding this comment.
in general what does the exclamation point after certain functions mean?
Couple questions for this line:
- what's stored in recv[0]
- What does the { .. } statement do
There was a problem hiding this comment.
- The exclamation point indicates that the function is actually a macro.
recvis aVec<MsgB2M>- The
MsgB2M::Unload(B2MUnload { .. }))is pattern matching syntax. https://doc.rust-lang.org/book/ch18-03-pattern-syntax.html
cjrkoa
reviewed
Nov 6, 2023
| return Err(anyhow::anyhow!("room already loaded")); | ||
| } | ||
| std::cmp::Ordering::Greater => { | ||
| // we have an newer version of this room, remove it | ||
| self.remove_room(&metadata.name, locator.monolith_id()) | ||
| self.unload_room(locator.monolith_id(), metadata.name.clone()) |
There was a problem hiding this comment.
difference between unload_room() and remove_room()?
There was a problem hiding this comment.
unload_room sends a message to the monolith that the room should be unloaded
remove_room modifies BalancerContext to remove the room from the balancer's state but doesn't send a message.
cjrkoa
reviewed
Nov 6, 2023
| @@ -343,12 +345,15 @@ impl BalancerContext { | |||
| match locator.load_epoch().cmp(&load_epoch) { | |||
| std::cmp::Ordering::Less => { | |||
| // we already have an older version of this room | |||
| self.unload_room(monolith_id, metadata.name.clone()).await?; | |||
| return Err(anyhow::anyhow!("room already loaded")); | |||
| } | |||
| std::cmp::Ordering::Greater => { | |||
There was a problem hiding this comment.
No, this code compares the load_epoch of the room. Only the room with the lowest load_epoch should be loaded, as specified in our spec doc.
cjrkoa
approved these changes
Nov 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
closes #1130