Page MenuHomeElementl

[dagit] RFC: useSubscription instead of DirectGraphQLSubscription

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



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

R1 dagster
Lint Not Applicable
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.


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

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.
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


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


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


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.

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