Page MenuHomeElementl

[dagit] RFC: useSubscription instead of DirectGraphQLSubscription
ClosedPublic

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

Details

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

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

dish requested review of this revision.Jul 30 2021, 6:06 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?

This revision is now accepted and ready to land.Jul 30 2021, 9:59 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

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.