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!

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

Commented line

Harbormaster completed remote builds in B34507: Diff 42668.Jul 30 2021, 7:53 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`.

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

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.

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.

[dagit] RFC: useSubscription instead of DirectGraphQLSubscription
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42837

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[dagit] RFC: useSubscription instead of DirectGraphQLSubscriptionClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42837

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[dagit] RFC: useSubscription instead of DirectGraphQLSubscription
ClosedPublic
Actions

Revision Contents
Changeset List