fix(plugin-meetings): moved cluster reachability functionality to new… #4580
Conversation
marcin-bazyl
added
the
validated
| latency: number, | ||
| publicIp?: string | null, | ||
| serverIp?: string | null, | ||
| reachedSubnets?: Set<string> |
There was a problem hiding this comment.
why is reachedSubnets optional? this function is called from only 1 place and with reachedSubnets always provided.
| * @param {Set<string>} [reachedSubnets] - Optional set to track reached subnets | ||
| * @returns {boolean} true if a new IP was added, false otherwise | ||
| */ | ||
| function processIceCandidateResult( |
There was a problem hiding this comment.
Some questions about this:
- I don't really understand the purpose of the refactor of existing ClusterReachability.saveResult() into the new ReachabilityPeerConnection.saveResult() and the free function processIceCandidateResult(). Can you explain the reasoning behind it?
- Why is processIceCandidateResult() not just a method inside ReachabilityPeerConnection?
- Why ReachabilityPeerConnection has reachedSubnets as a property? it's being added to, but never read from.
|
|
||
| this.reachabilityPeerConnection = new ReachabilityPeerConnection(clusterInfo, name); | ||
|
|
||
| this.reachabilityPeerConnection.on('resultReady', (data) => { |
There was a problem hiding this comment.
probably would be better to have an enum for all the ReachabilityPeerConnection events
| public readonly isVideoMesh: boolean; | ||
| public readonly name; | ||
| public readonly reachedSubnets: Set<string> = new Set(); | ||
| private result: ClusterReachabilityResult; |
There was a problem hiding this comment.
I don't think we need the result in ClusterReachability now. We have it inside ReachabilityPeerConnection and for now we only have a single instance of ReachabilityPeerConnection in every ClusterReachability, so there is no need to have a duplication of the result in the 2 classes. In one of your future PRs, maybe we'll have to introduce the result in ClusterReachability so that it will aggregate the results from all ReachabilityPeerConnection instances used, but right now ClusterReachability.getResult() can just return the result from this.reachabilityPeerConnection, right?
| private numTcpUrls: number; | ||
| private numXTlsUrls: number; | ||
| private result: ClusterReachabilityResult; | ||
| class ReachabilityPeerConnection extends EventsScope { |
There was a problem hiding this comment.
we usually try to have a single class in each file, so it's probably better to move this new class into a new file
|
|
||
| // Make sure that gatherIceCandidates is called before setLocalDescription | ||
| // as setLocalDescription triggers the ICE gathering process | ||
| assert.isTrue(gatherIceCandidatesSpy.calledBefore(fakePeerConnection.setLocalDescription)); |
There was a problem hiding this comment.
why has this check been removed?
2 participants
… class pr1
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
< DESCRIBE THE CONTEXT OF THE ISSUE >
by making the following changes
< DESCRIBE YOUR CHANGES >
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.