More comments in the signalling code #229
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
RoxyFarhad
force-pushed
the
roxy-comments
branch
from
75b196f to
bf0f446
Compare
viambot
added
safe to test
viambot
added
safe to test
RoxyFarhad
force-pushed
the
roxy-comments
branch
from
77f1b5c to
0afe9dd
Compare
viambot
added
safe to test
RoxyFarhad
force-pushed
the
roxy-comments
branch
from
0afe9dd to
9a547f7
Compare
viambot
added
safe to test
vijayvuyyuru
approved these changes
Jan 8, 2024
|
[minor] could you add your name to your comments to follow Eric's convention? I know people can use git blame but sometimes these are harder to follow. |
ohEmily
reviewed
Jan 8, 2024
rpc/wrtc_call_queue_mongodb.go
Outdated
| @@ -422,14 +424,13 @@ func (queue *mongoDBWebRTCCallQueue) changeStreamManager() { | |||
| } | |||
| }() | |||
| var lastSeq uint64 | |||
| var ranOnce bool | |||
| var ranOnce bool // this is only for the first time the changestream is setup | |||
There was a problem hiding this comment.
[minor] would it be worth renaming to initialized? or something else that documents that it only happens once.
There was a problem hiding this comment.
changed to isInitialized
ohEmily
reviewed
Jan 8, 2024
| @@ -597,15 +602,21 @@ func (queue *mongoDBWebRTCCallQueue) processNextSubscriptionEvent(next mongoutil | |||
| if _, ok := queue.csTrackingHosts[callResp.Host]; !ok { | |||
| // no one connected to this operator is currently subscribed to insert | |||
| // events for this host; skip | |||
| // we do this because each server is listening to an event and each host only lives on one server | |||
| // it could be on another server | |||
| return | |||
There was a problem hiding this comment.
would it be worth adding a debug log here or would it be so insanely noisy that it's not useful?
There was a problem hiding this comment.
i think it would be quite noisy and not useful
ohEmily
reviewed
Jan 8, 2024
rpc/wrtc_call_queue_mongodb.go
Outdated
| @@ -614,10 +625,12 @@ func (queue *mongoDBWebRTCCallQueue) processNextSubscriptionEvent(next mongoutil | |||
| // though, we want to send the events as fast as possible as mentioned | |||
| // above and cannot block on the send to see if the receiver locked | |||
| // the document. | |||
| queue.logger.Infow("sending event", "event", event) | |||
There was a problem hiding this comment.
[q] wouldn't this log once per channel without including identifying information for the channel? Would it make sense to instead log the event in the log line above at line 619?
(Apologies if I'm misunderstanding here!)
There was a problem hiding this comment.
I actually just think we should remove this here, it will be noisy and it was mainly for me debugging locally :)
RoxyFarhad
force-pushed
the
roxy-comments
branch
from
9a547f7 to
04d3883
Compare
viambot
added
safe to test
ohEmily
reviewed
Jan 9, 2024
.vscode/launch.json
Outdated
| @@ -0,0 +1,15 @@ | |||
| { | |||
viambot
added
safe to test
ohEmily
approved these changes
Jan 9, 2024
RoxyFarhad
force-pushed
the
roxy-comments
branch
from
309fe0a to
16a1ca9
Compare
viambot
added
safe to test
RoxyFarhad
force-pushed
the
roxy-comments
branch
from
1cbc8d8 to
33cd80d
Compare
viambot
added
safe to test
ohEmily
approved these changes
Jan 9, 2024
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.
No description provided.