fix(plugin-meetings): do not wait for upload logs before destroying meeting #3957
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-meetings/src/meetings/index.ts (1)
Line range hint
1252-1264: Consider extracting the log upload logic to reduce duplication.The log upload logic is duplicated between the
DESTROY_MEETINGandREQUEST_UPLOAD_LOGSevent handlers. Consider extracting it into a helper method:+private uploadMeetingLogs(meetingInstance: Meeting) { + if (this.config.autoUploadLogs) { + this.uploadLogs({ + callStart: meetingInstance?.locusInfo?.fullState?.lastActive, + locussessionid: meetingInstance?.locusInfo?.fullState?.sessionId, + correlationId: meetingInstance.correlationId, + feedbackId: meetingInstance.correlationId, + locusId: meetingInstance.locusId, + meetingId: meetingInstance.locusInfo?.info?.webExMeetingId, + autoupload: true, + }); + } +} createdMeeting.on(EVENTS.DESTROY_MEETING, (payload) => { - if (this.config.autoUploadLogs) { - this.uploadLogs({ - callStart: createdMeeting.locusInfo?.fullState?.lastActive, - locussessionid: createdMeeting.locusInfo?.fullState?.sessionId, - correlationId: createdMeeting.correlationId, - feedbackId: createdMeeting.correlationId, - locusId: createdMeeting.locusId, - meetingId: createdMeeting.locusInfo?.info?.webExMeetingId, - autoupload: true, - }); - } + this.uploadMeetingLogs(createdMeeting); this.destroy(createdMeeting, payload.reason); }); createdMeeting.on(EVENTS.REQUEST_UPLOAD_LOGS, (meetingInstance) => { - if (this.config.autoUploadLogs) { - this.uploadLogs({ - callStart: meetingInstance?.locusInfo?.fullState?.lastActive, - locussessionid: meetingInstance?.locusInfo?.fullState?.sessionId, - correlationId: meetingInstance.correlationId, - feedbackId: meetingInstance.correlationId, - locusId: meetingInstance.locusId, - meetingId: meetingInstance.locusInfo?.info?.webExMeetingId, - autoupload: true, - }); - } + this.uploadMeetingLogs(meetingInstance); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/src/meetings/index.ts(1 hunks)
🔇 Additional comments (1)
packages/@webex/plugin-meetings/src/meetings/index.ts (1)
1250-1250: LGTM! Log upload no longer blocks meeting destruction.
The changes correctly implement the PR objective by ensuring that log upload doesn't delay meeting destruction. This will prevent the reported 23-second delay when transitioning between meetings.
Also applies to: 1252-1252
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
brycetham
requested review from
szotrabh,
marcin-bazyl,
SomeBody16,
antsukanova,
k-wasniowski and
edvujic
COMPLETES SPARK-574076
This pull request addresses
When leaving a meeting, if the logs fail to upload (for example, if the log upload service is down), the meeting may not be destroyed until much later (up to 23 seconds after leaving the meeting in my tests). If the user rejoins the meeting during that time, it may cause them to get removed from the meeting when the meeting is destroyed.
by making the following changes
Do not wait for the logs to be uploaded before destroying the meeting.
Change Type
The following scenarios were tested
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
Summary by CodeRabbit