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.

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

Debounce the GraphQL subscription request call in 0.5 second #373

Closed
wants to merge 3 commits into from

Conversation

kinow
Copy link
Member

@kinow kinow commented Jan 18, 2020

This is a small change with no associated Issue.

The rationale for this PR, is that we avoid at least one unnecessary call to the server. At the moment, when a user visits the main page of Cylc UI, a dashboard view is displayed.

Due to the lifecycle of components/routes, we will get GScan being created and put on the layout. GScan is a component that also triggers subscriptions. After a while, other components are created, and finally the view is done, and the other lifecycle methods are called in the Dashboard view.

One of these lifecycle methods being created, where the view will register itself, and start a new subscription. This new subscription forces the WorkflowService to update the existing query created by GScan, and combine with the Dashboard query.

But when that happens, the GScan query will have triggered at least one request to the server. The UI will stop the GScan subscription, and start a new one for Dashboard, which is not optimal.

This PR uses Lodash's debounce, with 1 second timeout. Meaning that the function will be called, after nobody has called it for at least 1 second.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • No change log entry required (why? e.g. invisible to users).
  • No documentation update required.

…ecuted only

when there are no more calls after 1 second.
@kinow
Copy link
Member Author

kinow commented Jan 18, 2020

Needs a bit more of testing, and perhaps a unit test — if not too hard.

On master, once we open the UI we get:

image

Where the operatoin with ID 1 is not used, as it's stopped soon.

If you open another view, such as the GScan (table list) view, everything repeats, and we get another unnecessary operation.

image

In the image above, the current operation 2 (replaced the unused 1) gets stopped. Then an operation 3 is initialized, just to be stopped too. Then operation 4 is created.

Now with this branch.

image

Operation 1 is the only one, and is running with the combined GraphQL query.

Going to another view, such as the Workflow, we can again confirm only one operation was created.

image

The GScan query was subscribed. But then we can see that the query for the Tree component was registered afterwards, and is the one being used in the operation 2 — we know that, because the Tree includes the familyProxies, and without that information it cannot create the hierarchy, as well as confirming in the query sent to the server, as in the image below.

image

@kinow kinow self-assigned this Jan 18, 2020
@kinow kinow changed the title Debounce the request call in 1 second Debounce the GraphQL subscription request call in 1 second Jan 18, 2020
@kinow
Copy link
Member Author

kinow commented Jan 19, 2020

Tested again today, and it worked fine in the UI. You can quickly click on the "Dashboard" and on the "Workflow" links, and only the final link is activated, once you have not clicked for at least 1 second. Then it generates only a single WebSocket request/subscription.

@kinow kinow marked this pull request as ready for review January 20, 2020 00:49
@kinow
Copy link
Member Author

kinow commented Jan 20, 2020

Decreased the debounce timeout to 500 ms (per this thread). 1 second seemed too long, and the user could have to wait for the workflow to be displayed unnecessarily.

300ms was too short, and the browser would still submit 2 requests. 500ms works fine. I would prefer 200-300 ms (which I think is the limit of time UX suggests users can be made waiting for something to happen? I think), but because GScan components is triggering query/subscription, and that happens before views in Vue.js and VueRouter, we have no option but to delay a little longer :/

@kinow
Copy link
Member Author

kinow commented Jan 20, 2020

Added unit test, GitHub action passing, ready for review.

@kinow kinow added this to the 0.2 milestone Jan 20, 2020
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

LGTM

@kinow kinow changed the title Debounce the GraphQL subscription request call in 1 second Debounce the GraphQL subscription request call in 0.5 second Jan 22, 2020
@oliver-sanders
Copy link
Member

oliver-sanders commented Jan 29, 2020

Looks fine on the code front, I'm not sold on the debounce solution though, may need some convincing. If I understand correctly the aim is to solve these problems:

  1. The problem of multiple components initiating in quick succession i.e. when the user changes workflows.
    • With a debounce it will take two iterations before the subscriptions catch up with the request which is wasteful.
    • This situation is something we can detect (and potentially solve) in the Workflow component, perhaps a subscribeMany(subscriptions) interface on the workflowService would prevent the second subscription from being issued.
  2. The problem of multiple REST queries being fired in quick succession due to a user flicking about in the UI, opening lots of views or changing workflows.
    • Is this likely to be a problem?
    • Are users likely to open multiple views within the debounce period of 0.5 seconds?
    • It probably takes the user longer to click the "add view" button.

@kinow
Copy link
Member Author

kinow commented Jan 29, 2020

Moved to an issue: #388

@kinow kinow closed this Jan 29, 2020
@kinow kinow deleted the debounce-request branch March 19, 2020 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants