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

Remove query cancellation from StateManagers #3948

Merged
merged 5 commits into from
Feb 27, 2024

Conversation

bcolloran
Copy link
Contributor

experiment with removing query cancellation

Here's what I see in a couple conditions

(1) state as of changes in this PR:

  • all manually query cancellation removed
  • observeContextColumnWidth reactive statement commented out

Results:

  • everything above the fold loads in about 15 seconds,
  • loading keeps going for stuff below the fold
  • no queries cancelled
firefox_DpIsohhvGJ.mp4

(2) Main before this PR, but without observeContextColumnWidth

  • manual query cancellation in place
  • observeContextColumnWidth reactive statement commented out

Results:

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

It doesn't appear like these cancelQueries calls are adding any value to the dashboard. Out-of-the-box query cancellation works just fine without these explicit calls. Can you go ahead and remove the calls and related code?

However, for now can you keep the calls in ModelBody.svelte and resource-invalidations.ts? IIRC those are there as a ham-fisted way to cancel/invalidate outstanding queries.

@bcolloran
Copy link
Contributor Author

@AdityaHegde and @djbarnwal would love to get your takes on this also before we go forward with any changes.

@ericpgreen2 and I have been looking at query cancellation in the context of https://github.com/rilldata/rill-private-issues/issues/112, and think that we might be doing extra manual cancellations in places where tanstack should be able to do that automatically. I think that as a team, we need to audit all of those manual cancellation and understand why they are needed, and if they can be removed.

@AdityaHegde, I've assigned this to you because I think you probably know the most about the intersection between runtime and client here. I think "done" on this would be code comments for any places we want to retain manual query cancellation. I don't mind making the changes in this PR if you just want to put those comments as review comments.

@bcolloran bcolloran self-assigned this Jan 31, 2024
@djbarnwal
Copy link
Member

I haven't personally worked a lot with query cancellation and relied on tanstack for it. I am up for removing manual cancellation if they don't have any performance implications.

queryClient.cancelQueries({
predicate: (query) => isProfilingQuery(query, resource.meta.name.name),
});
// queryClient.cancelQueries({
Copy link
Collaborator

@AdityaHegde AdityaHegde Feb 13, 2024

Choose a reason for hiding this comment

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

We need these since there could be a race condition in request for profile sent and added to queue in backend vs the entity being deleted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same applies to line 158 and line 99 in ModelBody.

@bcolloran bcolloran changed the title comment out all instances of manual query cancellation Remove query cancellation from StateManagers (was: comment out all instances of manual query cancellation Feb 20, 2024
@bcolloran bcolloran marked this pull request as ready for review February 20, 2024 20:54
@bcolloran
Copy link
Contributor Author

@ericpgreen2 cleaning this up to get it over the line -- removed changes in ModelBody.svelte and resource-invalidations.ts, per you and @AdityaHegde's suggestions

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

Great. Looks like a good low-risk cleanup.

@ericpgreen2 ericpgreen2 changed the title Remove query cancellation from StateManagers (was: comment out all instances of manual query cancellation Remove query cancellation from StateManagers Feb 27, 2024
@ericpgreen2 ericpgreen2 merged commit 8a06da6 into main Feb 27, 2024
4 checks passed
@ericpgreen2 ericpgreen2 deleted the remove-query-cancellation branch February 27, 2024 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants