Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-24781 Make GM profiles API calls to be concurrent #5436

Merged
merged 3 commits into from
May 14, 2020

Conversation

sudheerDev
Copy link
Contributor

@sudheerDev sudheerDev commented May 4, 2020

Summary

  • Add p-queue as a dependency and keep concurrency at 4 calls.

On the web, we now make one call at a time for GM profiles as attached in the screenshot. This can be improved by making 4 calls at a time in parallel and adding a new request to the stack when one finishes.
Making multiple calls will fetch all profiles sooner and instead of blocking the network stack with all the requests at once, we can cap them at 4. The reason to cap them at 4 is to have a little room for other network requests to go through in case they are needed while these are pending.

Ticket Link

https://mattermost.atlassian.net/browse/MM-24781

Screenshots

Before
May-04-2020 19-03-10

After
May-05-2020 01-45-25

  * Add p-queue as a dependdency and keep concurrency at 4 calls
@sudheerDev sudheerDev added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels May 4, 2020
@sudheerDev sudheerDev added this to the v5.24.0 milestone May 4, 2020

await UserActions.loadProfilesForGM();
expect(UserActions.queue.onEmpty).toHaveBeenCalled();
expect(UserActions.queue.add).toHaveBeenCalled();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i wanted to assert that we are calling an action here but it was becoming hard to mock the class etc.

@@ -37,6 +37,7 @@
"marked": "github:mattermost/marked#5a06d1af25ca48927b9efe0d9c42c01c84e7839b",
"mattermost-redux": "github:mattermost/mattermost-redux#9a915443ab1d695540493406926c3dd269d380fc",
"moment-timezone": "0.5.27",
"p-queue": "^6.4.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The footprint of the library small and it has good API which I intend to use in upcoming features like prefetching posts. It has API's for clearing, pausing and adding tasks dynamically too which fits perfectly for some of the upcoming features.

Copy link
Contributor

@nevyangelova nevyangelova left a comment

Choose a reason for hiding this comment

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

Awesome and a very clean way to make an impactful change 👏

Copy link
Contributor

@Willyfrog Willyfrog left a comment

Choose a reason for hiding this comment

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

LGTM
added a minor change to keep the code cleaner

actions/user_actions.jsx Outdated Show resolved Hide resolved
Co-authored-by: Guillermo Vayá <[email protected]>
@Willyfrog Willyfrog removed the 2: Dev Review Requires review by a core commiter label May 5, 2020
@Willyfrog Willyfrog requested a review from jgilliam17 May 5, 2020 13:32
@sudheerDev sudheerDev added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label May 14, 2020
Copy link
Contributor

@jgilliam17 jgilliam17 left a comment

Choose a reason for hiding this comment

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

Thank you @sudheerDev
Tested, looks good to merge.

  • Verified up to 4 requests are made at a time when there are many GMs on the sidebar.
    Added test steps to the Jira ticket.

@sudheerDev
Copy link
Contributor Author

/update-branch

@jgilliam17 jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels May 14, 2020
@mm-cloud-bot
Copy link

Test server destroyed

@mm-cloud-bot
Copy link

New commit detected. SpinWick will upgrade if the updated docker image is available.

@mm-cloud-bot
Copy link

Test server creation failed. See the logs for more information.

@sudheerDev sudheerDev merged commit a11a5b7 into mattermost:master May 14, 2020
@sudheerDev sudheerDev deleted the MM-24781 branch May 14, 2020 17:55
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels May 14, 2020
@jgilliam17 jgilliam17 added the Tests/Done Release tests have been written label May 18, 2020
bradjcoughlin pushed a commit to bradjcoughlin/mattermost-webapp that referenced this pull request May 19, 2020
* MM-24781 Make GM profiles API calls to be concurrent

  * Add p-queue as a dependdency and keep concurrency at 4 calls

* Update actions/user_actions.jsx

Co-authored-by: Guillermo Vayá <[email protected]>

Co-authored-by: Guillermo Vayá <[email protected]>
Co-authored-by: mattermod <[email protected]>
bradjcoughlin pushed a commit to bradjcoughlin/mattermost-webapp that referenced this pull request May 19, 2020
* MM-24781 Make GM profiles API calls to be concurrent

  * Add p-queue as a dependdency and keep concurrency at 4 calls

* Update actions/user_actions.jsx

Co-authored-by: Guillermo Vayá <[email protected]>

Co-authored-by: Guillermo Vayá <[email protected]>
Co-authored-by: mattermod <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
7 participants