Page MenuHomeElementl
Differential D9161

[dagit] RFC: useSubscription instead of DirectGraphQLSubscription
ClosedPublic
Actions

Authored by • dish on Jul 30 2021, 4:53 PM.

Details

Reviewers
• bengotow
• prha
Commits
R1:c7c169e4dcfc: [dagit] RFC: useSubscription instead of DirectGraphQLSubscription
Summary

The other day I eliminated the WebSocketLink from our Apollo usage because we weren't using it -- everything was being done via DirectGraphQLSubscription.

In this diff, I propose replacing these callsites with Apollo's useSubscription, with fetchPolicy: 'no-cache' in place to try to avoid the performance problems that DirectGraphQLSubscription was intended to avoid. This allows us to clean up quite a bit of custom code, and will hopefully let us leverage Apollo's built-in WebSocket management (e.g. retries).

Test Plan

View run logs, compute logs. Verify that these stream properly, render properly. In the case of switching the visible step in compute logs, verify that the step is switched properly when using the step picker.

Use grpc and restart it. Verify that the warning message appears in the left nav.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• dish created this revision.Jul 30 2021, 4:53 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 30 2021, 5:07 PM
Harbormaster failed remote builds in B34488: Diff 42647!
• dish updated this revision to Diff 42662.Jul 30 2021, 5:55 PM

Mock the subscription value in RepositoryLocationStateObserver

Harbormaster completed remote builds in B34502: Diff 42662.Jul 30 2021, 6:06 PM
• dish requested review of this revision.Jul 30 2021, 6:06 PM
• dish updated this revision to Diff 42668.Jul 30 2021, 7:42 PM

Commented line

Harbormaster completed remote builds in B34507: Diff 42668.Jul 30 2021, 7:53 PM
• prha added a comment.Jul 30 2021, 9:49 PM

This makes sense to me... thanks for cleaning this up.

js_modules/dagit/packages/core/src/nav/RepositoryLocationStateObserver.tsx
33

is there a world where we would use a different policy, to make use of the fact that for completed runs, the responses will look the same?

• prha accepted this revision.Jul 30 2021, 9:59 PM
This revision is now accepted and ready to land.Jul 30 2021, 9:59 PM
• dish added inline comments.Aug 2 2021, 2:03 PM
js_modules/dagit/packages/core/src/nav/RepositoryLocationStateObserver.tsx
33

I think in some cases it might make sense, perhaps if we already have a run status that indicates that there will be no further logs. Though in those cases, we should probably just go ahead do a regular query anyway, since there's nothing to stream over WS.

In the case of this subscription (location state observer), I think we're always good with no-cache.

• dish updated this revision to Diff 42718.Aug 2 2021, 3:31 PM

Fix ref error in useViewport

Harbormaster completed remote builds in B34547: Diff 42718.Aug 2 2021, 3:43 PM
• bengotow accepted this revision.Aug 2 2021, 4:05 PM
• bengotow added inline comments.
js_modules/dagit/packages/core/src/gantt/useViewport.tsx
37 ↗(On Diff #42718)

This seems reasonable, but it also seems like this might be indicative of another problem somewhere else if it changed in this diff

js_modules/dagit/packages/core/src/nav/RepositoryLocationStateObserver.tsx
34

I wonder if we need to useCallback this? (Just a thought if things are behaving strangely)

js_modules/dagit/packages/core/src/runs/ComputeLogPanel.tsx
51–64

I think we could rm this fragment now that we have a wrapping div

js_modules/dagit/packages/core/src/runs/LogsProvider.tsx
52

I think this now returns the first pipeline run status transition and not the last! I think if it iterated backwards we'd be ok

• dish updated this revision to Diff 42833.Aug 9 2021, 7:42 PM

Use message batching in LogsProvider to ensure smooth Gantt animation.

• dish marked an inline comment as done.Aug 9 2021, 7:50 PM
• dish added inline comments.
js_modules/dagit/packages/core/src/nav/RepositoryLocationStateObserver.tsx
34

I think we're okay without a useCallback on this. But I'll keep it in mind if anything weird comes up.

• dish updated this revision to Diff 42836.Aug 9 2021, 7:51 PM

Extra fragment wrapper

Harbormaster completed remote builds in B34628: Diff 42833.Aug 9 2021, 7:56 PM
Harbormaster completed remote builds in B34629: Diff 42836.Aug 9 2021, 8:01 PM
Closed by commit R1:c7c169e4dcfc: [dagit] RFC: useSubscription instead of DirectGraphQLSubscription (authored by • dish). · Explain WhyAug 9 2021, 9:55 PM
This revision was automatically updated to reflect the committed changes.
• dish added a commit: R1:c7c169e4dcfc: [dagit] RFC: useSubscription instead of DirectGraphQLSubscription.

Revision Contents
Changeset List

Diff 42837

View Options

js_modules/dagit/packages/core/src/app/AppError.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/app/AppProvider.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/app/DirectGraphQLSubscription.ts

Loading...
View Options

js_modules/dagit/packages/core/src/gantt/GanttChart.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/gantt/RunGroupPanel.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/nav/LeftNav.test.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/nav/LeftNavRepositorySection.test.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/nav/RepositoryLocationStateObserver.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/runs/ComputeLogPanel.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/runs/ComputeLogProvider.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/runs/LogsProvider.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/runs/LogsRow.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/runs/RunMetadataProvider.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/runs/RunUtils.tsx

Loading...
View Options

js_modules/dagit/packages/core/src/runs/useComputeLogs.tsx

Loading...