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

Refresh button for source rows in Status page #6175

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

lovincyrus
Copy link
Contributor

@lovincyrus lovincyrus commented Nov 26, 2024

address #6154

  • invalidate resources data when refresh source is clicked
  • unable to invalidate individual source resource

CleanShot 2024-11-26 at 13 44 08@2x

@ericpgreen2
Copy link
Contributor

ericpgreen2 commented Nov 27, 2024

Note: we should also include the refresh button for Models because 1) a Model can be a root node of the DAG, 2) an intermediate model could still have logic that needs a refresh independent from an upstream node, and 3) we're soon deprecating the Source concept anyways.

And actually, if it's not too much a scope expansion, it'd be nice to be able to refresh Alerts & Reports too. See:
image

@lovincyrus
Copy link
Contributor Author

Note: we should also include the refresh button for Models because 1) a Model can be a root node of the DAG, 2) an intermediate model could still have logic that needs a refresh independent from an upstream node, and 3) we're soon deprecating the Source concept anyways.

And actually, if it's not too much a scope expansion, it'd be nice to be able to refresh Alerts & Reports too. See: image

Added row click refresh button to Source and Model resource kind. I also added a tooltip to indicate that a resource trigger will update all dependent resources.


Latest demo:

CleanShot.2024-12-03.at.17.27.54.mp4

@lovincyrus lovincyrus marked this pull request as ready for review December 4, 2024 01:30
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.

There's a lot of action when clicking the refresh button: flickering, jumpiness, table resizing. Some things I noticed:

  1. The Resources table unmounts temporarily. See Jam and screenshot:
    image
  2. The page gets auto-scrolled to the top (Jam), probably due to point 1.
  3. A RefreshTrigger resource is shown temporarily. Can we hide it from the list? Here's a screenshot from your video:
    image

@lovincyrus
Copy link
Contributor Author

Latest demo:

CleanShot.2024-12-04.at.12.26.40.mp4

@lovincyrus lovincyrus force-pushed the cyrus/trigger-individual-resources branch 2 times, most recently from d78933b to 22f6d01 Compare December 6, 2024 19:23
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.

Mostly looks good, but I ran into a weird state: ~50% of the time when refreshing a Source with an error, the source is marked "Idle", yet for about 30s the "Refresh all sources and models" button is disabled and I see polling ListResources network requests. Here's a Jam.

I also hit this 500 error, but I think it's been fixed on main, right? So it'd help if you could merge main into this branch.
image

scripts/tsc-with-whitelist.sh Outdated Show resolved Hide resolved
@lovincyrus lovincyrus force-pushed the cyrus/trigger-individual-resources branch from d27ca47 to 792276e Compare December 10, 2024 02:11
@lovincyrus
Copy link
Contributor Author

lovincyrus commented Dec 10, 2024

Mostly looks good, but I ran into a weird state: ~50% of the time when refreshing a Source with an error, the source is marked "Idle", yet for about 30s the "Refresh all sources and models" button is disabled and I see polling ListResources network requests. Here's a Jam.

I also hit this 500 error, but I think it's been fixed on main, right? So it'd help if you could merge main into this branch. image

I can see from the Jam that the specific resource didn't get refreshed properly due to the "failed to ingest source" error. Here's a video of a happy path:

CleanShot.2024-12-10.at.10.32.21.mp4

Is it expected to disable the refresh icon when a resource errors out? I can see in the Jam, the resource continues to error out even after a refresh..

Update:

Gracefully stop polling after detecting reconcile error

CleanShot.2024-12-10.at.14.36.28.mp4

@lovincyrus
Copy link
Contributor Author

I also hit this 500 error, but I think it's been fixed on main, right? So it'd help if you could merge main into this branch.
image

After a rebase, I still run into the 500 error occasionally when I fire a $createTrigger call from the individual rows. This doesn't need to block this PR. Would you have more context? @begelundmuller

Error message — "grpc: error while marshaling: marshaling rill.runtime.v1.ListResourcesResponse: size mismatch (see golang/protobuf#1609): calculated=229, measured=177"

CleanShot 2024-12-10 at 14 01 21@2x

@begelundmuller
Copy link
Contributor

begelundmuller commented Dec 10, 2024

After a rebase, I still run into the 500 error occasionally when I fire a $createTrigger call from the individual rows. This doesn't need to block this PR. Would you have more context? @begelundmuller

Yeah that should get fixed by this PR: #6233

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.

UXQA: I've triggered a Source refresh, yet I don't see any polling of the ListResource API.

Kapture.2024-12-16.at.14.40.05.mp4

Comment on lines +41 to +43
onError: () => {
stopPolling();
},
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. onError will be deprecated in TanStack Query v5 (see here)
  2. Rather than the imperative startPolling(), stopPolling(), have you considered using the built-in declarative refetchInterval: () => number | false?

@lovincyrus
Copy link
Contributor Author

UXQA: I've triggered a Source refresh, yet I don't see any polling of the ListResource API.

Kapture.2024-12-16.at.14.40.05.mp4

In this PR, I've used the ListResources API to refresh a resource.

Taking another look at this,
image

Specifically, the Refresh a model command.
CleanShot 2024-12-18 at 23 53 52@2x

I see runtimev1.RefreshModelTrigger and runtimev1.CreateTriggerRequest from the refresh.go. Any reason why we don't have RefreshModelTrigger runtime service? @ericpgreen2

@begelundmuller
Copy link
Contributor

begelundmuller commented Dec 27, 2024

@lovincyrus @ericpgreen2 FYI RefreshModelTrigger is used in CreateTriggerRequest.models (link) to enable fine-grained refreshes of models (incremental/full refreshes, partition refreshes, retries of errored partitions, etc.). You can still use specify a model in CreateTriggerRequest.resources (link), but then it will always be a "normal" model refresh (which is a full refresh for non-incremental models and an incremental refresh for incremental models).

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.

3 participants