make balancer ids constant from the perspective of the monolith #1411

Victor-M-Giraldo · 2024-02-27T20:46:12Z

coderabbitai · 2024-02-27T20:46:16Z

Important

Auto Review Skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit-tests for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

codecov · 2024-02-27T20:51:37Z

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 56.1352%. Comparing base (06f58dc) to head (0fb9118).

Files	Patch %	Lines
server/generated.ts	0.0000%	6 Missing ⚠️
server/clientmanager.ts	0.0000%	5 Missing ⚠️
server/balancer.ts	0.0000%	2 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##             master      #1411        +/-   ##
================================================
- Coverage   56.1624%   56.1352%   -0.0272%     
================================================
  Files           163        163                
  Lines         24828      24840        +12     
  Branches       1432       1432                
================================================
  Hits          13944      13944                
- Misses        10830      10842        +12     
  Partials         54         54

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dyc3 · 2024-02-29T01:14:30Z

I also added a BalancerId type in #1416. You should rebase this

Victor-M-Giraldo · 2024-03-02T01:51:09Z

Since you fixed the commit history I just have a bunch of unresolved import errors. I'm not sure if that is only on my end.

I also just realized I can't do this inside this function since it needs to be async to await it. Would making this an async function break functionality? Does it even make sense to send the balancer id when a monolith is added? It kind of does to me. Will also try and fix the test if this is actually the right way to go about this.

dyc3

The balancer id needs to be global and constant throughout the time the balancer is alive. In other words, the balancer must generate the id once and once only. All areas where the id is accessed should return the initial value. I'm expecting something along the lines of static x = Lazy<...> from once_cell somewhere in here.

server/clientmanager.ts

crates/ott-balancer/src/balancer.rs

Victor-M-Giraldo · 2024-03-09T16:31:28Z

Okay, not a clue on how to actually send this. I've been taking a look at this

dyc3 · 2024-03-09T16:39:42Z

WebSocketStream impls Sink<Message>: https://docs.rs/hyper-tungstenite/latest/hyper_tungstenite/struct.WebSocketStream.html#impl-Sink%3CMessage%3E-for-WebSocketStream%3CT%3E

send is impled by the SinkExt blanket impl: https://docs.rs/hyper-tungstenite/latest/hyper_tungstenite/struct.WebSocketStream.html#impl-SinkExt%3CItem%3E-for-T

We already send messages on WebSocketStreams. Use those as examples.

opentogethertube/crates/ott-balancer/src/client.rs

Line 202 in 061229c

if let Err(err) = stream.send(msg).await {

Victor-M-Giraldo · 2024-03-09T17:48:31Z

Okay, will take a look now.

Victor-M-Giraldo · 2024-03-11T05:25:31Z

Know this one is going to fail as well but I feel like this is closer. Ideally I want to send it in that loop right after what I wrote. I was trying to do have another stream.next() but I can't have multiple mutable borrows. Do you think that is the right direction?

dyc3

Yeah this is the right direction

crates/ott-balancer/src/balancer.rs

crates/ott-balancer/src/connection.rs

Victor-M-Giraldo · 2024-03-12T16:54:14Z

balancer stderr: console subscriber server failed: tonic::transport::Error(Transport, hyper::Error(Listen, Os { code: 98, kind: AddrInUse, message: "Address already in use" }))

I think this is the relevant part of why the tests are failing. Could you take a look?

dyc3 · 2024-03-12T16:58:19Z

console_subscriber failing to bind to it's port will not kill the balancer. The harness tests are just really flaky in CI.

Victor-M-Giraldo · 2024-03-12T17:06:39Z

I run them locally as well and the same two don't pass consistently. Otherwise, if you think it isn't relevant/not an issue I think this one might be ready.

dyc3 · 2024-03-12T17:50:01Z

The problem is with the harness tests. Since the balancer now sends a message to the monolith on connect the m.wait_recv() call returns immediately without waiting to receive the client join message.

This appears to make a lot of harness tests fail consistently on my machine

dyc3 · 2024-03-12T19:14:09Z

rebase this PR to fix the tests

Victor-M-Giraldo · 2024-03-12T19:18:05Z

Okay, will do.

Victor-M-Giraldo · 2024-03-12T20:34:05Z

Could you do a rerun on the rust tests?

Victor-M-Giraldo · 2024-03-12T23:47:20Z

Think this one is good to go. Only thing is the messages get double logged See here.. I see why, but what would the logging get changed into in the clientmanager.ts file?

Also, I guess I screwed up the rebase this time as well. It did tell me it worked, but then I had the two new commits you added as pending to be pushed (Since I wanted to be able to rerun the CI stuff). When that happens do you just force push?

dyc3 · 2024-03-13T11:33:25Z

yes, after a rebase, force push

dyc3

Looks good.

cypress · 2024-03-13T15:21:37Z

Passing run #1291 ↗︎

0	75	1	0	0

Details:

make balancer ids constant from the perspective of the monolith (#1411)
Project: OpenTogetherTube	Commit: `3c19f1ad62`
Status: Passed	Duration: 04:08 💡
Started: Mar 13, 2024 3:17 PM	Ended: Mar 13, 2024 3:21 PM

Review all test suite changes for PR #1411 ↗︎

Victor-M-Giraldo changed the title ~~make balancer ids constant~~ make balancer ids constant from the perspective of the monolith Feb 27, 2024

dyc3 force-pushed the make-balancer-ids-constant branch from aac606b to 13b0135 Compare March 1, 2024 20:10

Victor-M-Giraldo marked this pull request as ready for review March 2, 2024 02:07

Victor-M-Giraldo requested a review from dyc3 as a code owner March 2, 2024 02:07

dyc3 requested changes Mar 2, 2024

View reviewed changes

server/clientmanager.ts Show resolved Hide resolved

crates/ott-balancer/src/balancer.rs Outdated Show resolved Hide resolved

crates/ott-balancer/src/balancer.rs Outdated Show resolved Hide resolved

Victor-M-Giraldo marked this pull request as draft March 2, 2024 16:55

dyc3 reviewed Mar 11, 2024

View reviewed changes

crates/ott-balancer/src/balancer.rs Outdated Show resolved Hide resolved

crates/ott-balancer/src/connection.rs Outdated Show resolved Hide resolved

dyc3 mentioned this pull request Mar 12, 2024

improve harness test assertions to be more resilient to protocol changes #1485

Merged

Victor-M-Giraldo force-pushed the make-balancer-ids-constant branch from b16def7 to 518d7e7 Compare March 12, 2024 20:01

Victor-M-Giraldo marked this pull request as ready for review March 12, 2024 23:44

Victor-M-Giraldo requested a review from dyc3 March 13, 2024 14:37

dyc3 approved these changes Mar 13, 2024

View reviewed changes

Add BalancerId field to B2MLoad struct

6429c64

Victor-M-Giraldo added 16 commits March 13, 2024 10:53

add B2MInit

3defd5a

added trait implementations to b2minit

ab1f56e

Add BalancerId field to B2MLoad struct

4a6080e

add B2MInit

d652594

send B2MInit when a monolith is added.

b7adaae

fix lint and made join_monolith async

647fd23

fix ci?

64f1aaa

make balancer id global

586ba8f

log init message

2020137

get rid of old attempt

7eb7b7a

another attempt

ae0cc0b

fully revert changes

5172937

move to before monolith sends init

f8ed87b

update balancer id in balancerconn

e2d3143

properly log init message from balancer

1afa2b4

fix lint

4d5fd93

dyc3 force-pushed the make-balancer-ids-constant branch from e3a170d to 4d5fd93 Compare March 13, 2024 14:53

dyc3 enabled auto-merge (squash) March 13, 2024 14:54

dyc3 disabled auto-merge March 13, 2024 14:54

format

0fb9118

dyc3 merged commit 3c19f1a into dyc3:master Mar 13, 2024
19 of 20 checks passed

Victor-M-Giraldo deleted the make-balancer-ids-constant branch March 13, 2024 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make balancer ids constant from the perspective of the monolith #1411

make balancer ids constant from the perspective of the monolith #1411

Victor-M-Giraldo commented Feb 27, 2024

coderabbitai bot commented Feb 27, 2024 •

edited

Loading

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

codecov bot commented Feb 27, 2024 •

edited

Loading

dyc3 commented Feb 29, 2024

Victor-M-Giraldo commented Mar 2, 2024 •

edited

Loading

dyc3 left a comment •

edited

Loading

Victor-M-Giraldo commented Mar 9, 2024 •

edited by dyc3

Loading

dyc3 commented Mar 9, 2024 •

edited

Loading

Victor-M-Giraldo commented Mar 9, 2024

Victor-M-Giraldo commented Mar 11, 2024

dyc3 left a comment

Victor-M-Giraldo commented Mar 12, 2024

dyc3 commented Mar 12, 2024

Victor-M-Giraldo commented Mar 12, 2024

dyc3 commented Mar 12, 2024 •

edited

Loading

dyc3 commented Mar 12, 2024

Victor-M-Giraldo commented Mar 12, 2024

Victor-M-Giraldo commented Mar 12, 2024

Victor-M-Giraldo commented Mar 12, 2024 •

edited

Loading

dyc3 commented Mar 13, 2024

dyc3 left a comment

cypress bot commented Mar 13, 2024

make balancer ids constant from the perspective of the monolith #1411

make balancer ids constant from the perspective of the monolith #1411

Conversation

Victor-M-Giraldo commented Feb 27, 2024

coderabbitai bot commented Feb 27, 2024 • edited Loading

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

codecov bot commented Feb 27, 2024 • edited Loading

Codecov Report

dyc3 commented Feb 29, 2024

Victor-M-Giraldo commented Mar 2, 2024 • edited Loading

dyc3 left a comment • edited Loading

Choose a reason for hiding this comment

Victor-M-Giraldo commented Mar 9, 2024 • edited by dyc3 Loading

dyc3 commented Mar 9, 2024 • edited Loading

Victor-M-Giraldo commented Mar 9, 2024

Victor-M-Giraldo commented Mar 11, 2024

dyc3 left a comment

Choose a reason for hiding this comment

Victor-M-Giraldo commented Mar 12, 2024

dyc3 commented Mar 12, 2024

Victor-M-Giraldo commented Mar 12, 2024

dyc3 commented Mar 12, 2024 • edited Loading

dyc3 commented Mar 12, 2024

Victor-M-Giraldo commented Mar 12, 2024

Victor-M-Giraldo commented Mar 12, 2024

Victor-M-Giraldo commented Mar 12, 2024 • edited Loading

dyc3 commented Mar 13, 2024

dyc3 left a comment

Choose a reason for hiding this comment

cypress bot commented Mar 13, 2024

Passing run #1291 ↗︎

Review all test suite changes for PR #1411 ↗︎

coderabbitai bot commented Feb 27, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

codecov bot commented Feb 27, 2024 •

edited

Loading

Victor-M-Giraldo commented Mar 2, 2024 •

edited

Loading

dyc3 left a comment •

edited

Loading

Victor-M-Giraldo commented Mar 9, 2024 •

edited by dyc3

Loading

dyc3 commented Mar 9, 2024 •

edited

Loading

dyc3 commented Mar 12, 2024 •

edited

Loading

Victor-M-Giraldo commented Mar 12, 2024 •

edited

Loading