Page MenuHomePhabricator

bypass apollo for compute log subscriptions
ClosedPublic

Authored by prha on Wed, Oct 2, 11:09 PM.

Details

Reviewers
bengotow
alangenfeld
Group Reviewers
Restricted Project
Commits
R1:e8a05db2b575: bypass apollo for compute log subscriptions
Summary

This diff achieves the following:

  • Strips out the Apollo client / query / subscription
  • Adds the ComputeLogProvider which handles subscription messages, merging them before passing to the presentation component
  • Moves ComputeLogContent and other presentation only components to a separate file
  • Adds graphql fragments to the presentation layer
  • Uses generated fragment types instead of bespoke hand-written types for props
Test Plan

bk, ran stdout_spew_pipeline, saw streaming logs

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

prha created this revision.Wed, Oct 2, 11:09 PM
prha edited the summary of this revision. (Show Details)Wed, Oct 2, 11:22 PM
prha added reviewers: Restricted Project, bengotow.
alangenfeld accepted this revision.Thu, Oct 3, 2:48 PM
alangenfeld added a subscriber: alangenfeld.
alangenfeld added inline comments.
js_modules/dagit/src/plan/ComputeLogModal.tsx
206–213

while this is cute - as the stdout strings grow to be several MB is this going to be an issue?

This revision is now accepted and ready to land.Thu, Oct 3, 2:48 PM
prha updated this revision to Diff 5482.Thu, Oct 3, 3:33 PM
prha edited the summary of this revision. (Show Details)

clean up subscription merge, get rid of unused query

prha updated this revision to Diff 5483.Thu, Oct 3, 3:35 PM

remove unused type file

bengotow accepted this revision.Thu, Oct 3, 4:38 PM
bengotow added inline comments.
js_modules/dagit/src/plan/ComputeLogContent.tsx
110

Wow I actually didn't know you could say <div content={'text'} />.

I think the performance of this is probably good enough (and not the highest priority to fix), but I think it'd be slightly preferable to keep the string chunks in an array and append new divs to a container rather than replacing the existing string, since some react keys could keep the existing divs untouched and the browser could avoid re-running text layout on everything but the latest chunk.

Would probably require that the chunks are always split at the end of a line though.

193–194

Hmm this is a minor nit but it's much faster to do props.content.match(/\n/g).length since it doesn't build an array of the strings!

js_modules/dagit/src/plan/ComputeLogModal.tsx
206–213

Hmm I'm not sure what the perf overhead of this is, but it does seem like it's probably slightly slower than accumulating the strings into an array. Does the UI layer split these strings into separate HTML entities per line? If so we might want to do the splitting here (rather than joining the strings and then splitting it all).

prha updated this revision to Diff 5495.Thu, Oct 3, 5:38 PM

rebase

This revision was automatically updated to reflect the committed changes.