Page MenuHomePhabricator

Substantially reduce fetching required to get the entire schema
ClosedPublic

Authored by schrockn on Dec 8 2019, 3:57 AM.

Details

Summary

This changes the large environment schema fetch when
the execution console is loaded to not required the querying of
innerTypes. This reduces payload for large schemas dramatically.

Also added a configTypeKey field to Field to avoid one layer
of objects in js and python layer as well as the Apollo-injected
__typename field.

Depends on D1557
Depends on D1599

Resolves https://github.com/dagster-io/dagster/issues/1959

Test Plan

BK. Load dagit. Use config editor on large pipeline.

See schema load in around 5 secs and editor load and parse 15 seconds
later

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

schrockn created this revision.Dec 8 2019, 3:57 AM
schrockn updated this revision to Diff 7311.Dec 8 2019, 9:00 PM
schrockn retitled this revision from Sketchy of config editor perf improvements to Substantially reduce fetching required to get the entire schema.
schrockn edited the summary of this revision. (Show Details)
schrockn edited the test plan for this revision. (Show Details)

upmessage

Query size before:

Query size after:

js_modules/dagit/src/__tests__/ConfigTypeSchema.test.tsx
0

Are these really worth maintaining?

schrockn updated this revision to Diff 7312.Dec 8 2019, 9:02 PM
schrockn added reviewers: alangenfeld, bengotow.

upmessage

Harbormaster failed remote builds in B5908: Diff 7312!
schrockn edited the summary of this revision. (Show Details)Dec 9 2019, 10:00 PM
alangenfeld resigned from this revision.Dec 10 2019, 3:56 PM

I'm going to let @bengotow take this one

js_modules/dagit/src/configeditor/codemirror-yaml/mode.tsx
494–495

i see this refers to [1]

501–504

[1]

bengotow accepted this revision.Dec 10 2019, 6:50 PM

Oh man this looks great to me. Really glad we were able to get innerTypes out of here and the new interfaces for the ConfigTypeSchema component look great. Ship it!

js_modules/dagit/src/ConfigTypeSchema.tsx
135

Ahh this is a really nice addition 👍

js_modules/dagit/src/__tests__/ConfigTypeSchema.test.tsx
0

Yeah I really don't feel strongly—I think they really just test that renderTypeRecursive method, and it's fairly obvious when that is broken..

This revision is now accepted and ready to land.Dec 10 2019, 6:50 PM
schrockn updated this revision to Diff 7485.Dec 10 2019, 8:52 PM

delete annoying test