-
Notifications
You must be signed in to change notification settings - Fork 397
fix(breakout): keep history csis for meeting members #5023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Changes from 3 commits
a9eaa2b
7d643c4
b32a862
520702c
d7e6af9
948d193
d9ff599
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| /*! | ||
| * Copyright (c) 2015-2020 Cisco Systems, Inc. See LICENSE file. | ||
| */ | ||
| import {get, isEmpty, set} from 'lodash'; | ||
| import {get, isEmpty, isEqual, set} from 'lodash'; | ||
| // @ts-ignore | ||
| import {StatelessWebexPlugin} from '@webex/webex-core'; | ||
|
|
||
|
|
@@ -15,6 +15,7 @@ import { | |
| } from '../constants'; | ||
| import Trigger from '../common/events/trigger-proxy'; | ||
| import Member from '../member'; | ||
| import MemberUtil from '../member/util'; | ||
| import LoggerProxy from '../common/logs/logger-proxy'; | ||
| import ParameterError from '../common/errors/parameter'; | ||
| import { | ||
|
|
@@ -99,6 +100,14 @@ export default class Members extends StatelessWebexPlugin { | |
| selfId: any; | ||
| type: any; | ||
|
|
||
| /** | ||
| * Map of memberId -> CSIs that the member used in previous Locus updates. | ||
| * Kept here (rather than on each Member) so it survives members being removed | ||
| * and re-added (e.g. when entering/leaving a breakout session). | ||
| * @private | ||
| */ | ||
| private historyCsisByMemberId: Map<string, Set<number>> = new Map(); | ||
|
|
||
| namespace = MEETINGS; | ||
|
|
||
| /** | ||
|
|
@@ -569,6 +578,17 @@ export default class Members extends StatelessWebexPlugin { | |
| */ | ||
| private removeMembers(removedMembers: Array<string>) { | ||
| removedMembers.forEach((memberId) => { | ||
| // capture CSIs before removal so findMemberByCsi can still resolve them | ||
| // if the member is later re-added (e.g. when leaving a breakout session) | ||
| // with a participant payload that no longer contains the original CSIs | ||
| const existingMember = this.membersCollection.get(memberId); | ||
| if (existingMember) { | ||
| const history = new Set<number>(); | ||
| MemberUtil.extractCsis(existingMember.participant).forEach((csi) => history.add(csi)); | ||
| if (history.size > 0) { | ||
| this.historyCsisByMemberId.set(memberId, history); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fresh evidence beyond the earlier thread is that this newly added removal path overwrites any existing CSI history for the member. When a member already has cached CSIs from an earlier update and the later Useful? React with 👍 / 👎.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we're overwriting the history entry here, instead we should be adding new CSIs to it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't this introduce redundant data? If a signed-in user joins the meeting again, the CSI generated during the first join would already be invalid. I'm not sure whether an expired CSI could later be reused by another participant.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, actually, you're right, if CSIs change we shouldn't really need the old ones for anything. |
||
| } | ||
| } | ||
| this.membersCollection.remove(memberId); | ||
| }); | ||
| } | ||
|
|
@@ -598,6 +618,21 @@ export default class Members extends StatelessWebexPlugin { | |
| set(member, prop, existingValue); | ||
| } | ||
| }); | ||
|
|
||
| // remember CSIs the member used previously so findMemberByCsi can still | ||
| // resolve a CSI even after participant.devices[].csis no longer contains it. | ||
| // Skip re-extracting when devices haven't changed - existing history already covers them. | ||
| const devicesUnchanged = isEqual( | ||
| existingMember.participant?.devices, | ||
| member.participant?.devices | ||
| ); | ||
| if (!devicesUnchanged) { | ||
| const history = new Set<number>(); | ||
| MemberUtil.extractCsis(existingMember.participant).forEach((csi) => history.add(csi)); | ||
| if (history.size > 0) { | ||
| this.historyCsisByMemberId.set(member.id, history); | ||
|
WeijuanShao marked this conversation as resolved.
Outdated
WeijuanShao marked this conversation as resolved.
Outdated
marcin-bazyl marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| } | ||
| } | ||
| this.membersCollection.set(member.id, member); | ||
|
|
@@ -1161,18 +1196,38 @@ export default class Members extends StatelessWebexPlugin { | |
| ); | ||
| } | ||
|
|
||
| /** Finds a member that has any device with a csi matching provided value | ||
| /** Finds a member that has any device with a csi matching provided value. | ||
| * Falls back to the `historyCsisByMemberId` map so that a CSI a member used in | ||
| * a previous Locus update can still be resolved even if it is no longer | ||
| * present in `participant.devices[].csis`. | ||
| * | ||
| * @param {number} csi | ||
| * @returns {Member} | ||
| */ | ||
| findMemberByCsi(csi) { | ||
| return Object.values(this.membersCollection.getAll()).find((member) => | ||
| const members = Object.values(this.membersCollection.getAll()); | ||
|
|
||
| const currentMatch = members.find((member) => | ||
| // @ts-ignore | ||
| member.participant?.devices?.find((device) => | ||
| device.csis?.find((memberCsi) => memberCsi === csi) | ||
| ) | ||
| ); | ||
|
|
||
| if (currentMatch) { | ||
| return currentMatch; | ||
| } | ||
|
|
||
| for (const [memberId, history] of this.historyCsisByMemberId) { | ||
| if (history.has(csi)) { | ||
| const member = this.membersCollection.get(memberId); | ||
|
marcin-bazyl marked this conversation as resolved.
Outdated
|
||
| if (member) { | ||
| return member; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: we're only using this to find a member for a specific csi, so maybe instead of having a map of sets we could just have a map where the keys are CSI and values are memberId? It would also simplify the code:
for storing it, we would just call:
historyCsisByMemberId.set(csi, member.id)and that's it no other checks or code needed
and for reading it:
memberId=historyCsisByMemberId.get(csi)and that's it, no for loop or anything else
what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that approach as well. Personally, I feel that using memberId as the key provides a more member-centric view of the data, which makes the overall data model clearer.
That said, I agree that using the data can be more convenient when csi is the key. If you think using csi as the key is the better approach, I'm happy to switch to that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main downside of current approach is that we have to loop over all the entries in
findMemberByCsi()- when we have a 1k meeting or in the future 5k meetings in theory we may have to be doing a loop over thousands of members andfindMemberByCsi()is called whenever the person shown in one of video panes changes, so that can be quite often in a large meeting where more than 6 people are speaking (although maybe in practice not all members will have entries in historyCsisByMemberId?), so that's the main reason why I think using csi as key would be better.