Page MenuHomeElementl

[dagit] Move WebSocket context

Authored by dish on Jul 27 2021, 6:33 PM.
Referenced Files
Unknown Object (File)
Sun, Feb 5, 3:07 AM
Unknown Object (File)
Thu, Feb 2, 9:50 PM
Unknown Object (File)
Tue, Jan 24, 6:10 AM
Unknown Object (File)
Sun, Jan 15, 7:30 AM
Unknown Object (File)
Sat, Jan 14, 6:48 AM
Unknown Object (File)
Jan 5 2023, 1:38 PM
Unknown Object (File)
Dec 27 2022, 12:12 AM
Unknown Object (File)
Dec 19 2022, 7:05 AM



Stacked on

  • Remove WebSocketLink from AppProvider, move SubscriptionClient into WebSocketProvider.
  • Modify context being provided by WebSocketProvider (which I renamed btw) so that it includes all the important stuff needed to set up WebSocket connections, notably connectionParams and websocketURI
Test Plan

Add some headers to src/index, view runs in Dagit. Verify that WS connections now include those values as connection params on initialization.

Verify that WS connections are working as expected, including the connection status.

Diff Detail

R1 dagster
Lint Not Applicable
Tests Not Applicable

Event Timeline

websocketURI is always gonna be there

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 27 2021, 6:43 PM
Harbormaster completed remote builds in B34291: Diff 42388.
Harbormaster failed remote builds in B34293: Diff 42391!

Kick bk after setting parent

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 27 2021, 7:11 PM
Harbormaster failed remote builds in B34299: Diff 42398!

This looks good to me! Added a couple inline comments but I think this is a nice improvement.


It looks like we're counting on the identity of headers not changing for the useMemo on WebSocketProvider::48. I /think/ thats true here but would be good to check that this isn't parsed on each render from JSON or something...


It looks like we're opening a websocket connection here just to track it's status and do the status dot?

It seems like a good idea to open a websocket just to know that it /will/ work ok when we need to retrieve run logs, etc., but if we wanted to get rid of this and the status dot that also seems like it'd be fine at this point.

This revision is now accepted and ready to land.Jul 27 2021, 7:44 PM

Yeah, I can do a stringify on this to sanity check.


Yeah, we're still using the status value to determine whether to show the lost websocket warning on the Gantt chart. Not sure if it's being used in many other places, except for the dot.

We can revisit in a followup.

Stringify to ensure we keep the same header object

This revision was automatically updated to reflect the committed changes.