Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor(plugin-meetings): conditional stream addition for MQA #3610

Merged
merged 17 commits into from
Jul 3, 2024
Merged

refactor(plugin-meetings): conditional stream addition for MQA #3610

merged 17 commits into from
Jul 3, 2024

Conversation

edvujic
Copy link
Contributor

@edvujic edvujic commented May 22, 2024

COMPLETES WEBEX-378949

This pull request introduces enhancements to MQA, particularly focusing on the logic that determines which streams are sent to the MQA based on their isRequested status.

Change Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tooling change
  • Internal code refactor

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

@edvujic edvujic self-assigned this May 22, 2024
@edvujic edvujic requested a review from a team as a code owner May 22, 2024 18:14
@edvujic edvujic force-pushed the is-requested-stream branch from e2b891e to afb9e56 Compare May 22, 2024 18:14
@aws-amplify-us-east-2 AWS Amplify (us-east-2)
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-3610.d3m3l2kee0btzx.amplifyapp.com

mkesavan13
mkesavan13 requested changes May 23, 2024
View reviewed changes
Copy link
Contributor

@mkesavan13 mkesavan13 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes mostly look good. Just a few comments on the test. I've added it now but please do add "validated" tag once you run the tests locally and they pass so that the CI will run them. Unless the "validated" tag is added, CI won't run the tests

packages/@webex/plugin-meetings/test/unit/spec/stats-analyzer/index.js Outdated Show resolved Hide resolved
packages/@webex/plugin-meetings/test/unit/spec/stats-analyzer/index.js Show resolved Hide resolved
@mkesavan13 mkesavan13 added the validated If the pull request is validated for automation. label May 23, 2024
@edvujic edvujic requested a review from mkesavan13 May 23, 2024 16:15
mkesavan13
mkesavan13 approved these changes May 24, 2024
View reviewed changes
Copy link
Contributor

@mkesavan13 mkesavan13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you.

Please update the branch before merging

@edvujic edvujic requested a review from a team as a code owner June 20, 2024 12:05
@edvujic edvujic requested a review from antsukanova June 20, 2024 12:05
SomeBody16
SomeBody16 approved these changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@SomeBody16 SomeBody16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@edvujic edvujic requested a review from antsukanova June 20, 2024 13:58
@edvujic edvujic requested a review from antsukanova June 24, 2024 10:07
antsukanova
antsukanova approved these changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@antsukanova antsukanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for adding tests cases

@edvujic edvujic requested a review from brycetham June 28, 2024 08:32
brycetham
brycetham reviewed Jun 28, 2024
View reviewed changes
Copy link
Contributor

@brycetham brycetham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Having the helper util function for isStreamRequested is a good idea. Once the unit tests are fixed (the nested it block issue that we discussed offline) I can approve.

@edvujic edvujic requested a review from brycetham July 3, 2024 14:10
brycetham
brycetham approved these changes Jul 3, 2024
View reviewed changes
packages/@webex/plugin-meetings/src/statsAnalyzer/index.ts
Comment on lines +1020 to +1021
this.statsResults[mediaType][sendrecvType].retransmittedBytesSent =
result.retransmittedBytesSent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you may have accidentally added these lines in here from another task but that should be ok :)

@edvujic edvujic merged commit 3c72eb3 into webex:next Jul 3, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antsukanova antsukanova antsukanova approved these changes

@mkesavan13 mkesavan13 mkesavan13 approved these changes

@brycetham brycetham brycetham approved these changes

@SomeBody16 SomeBody16 SomeBody16 approved these changes

@szotrabh szotrabh Awaiting requested review from szotrabh

@bbaldino bbaldino Awaiting requested review from bbaldino

@k-wasniowski k-wasniowski Awaiting requested review from k-wasniowski

Assignees

@edvujic edvujic

Labels
validated If the pull request is validated for automation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@edvujic @mkesavan13 @brycetham @SomeBody16 @antsukanova