Page MenuHomePhabricator

Change compute logs graphql endpoint to query stdout/stderr separately
ClosedPublic

Authored by prha on Fri, Oct 4, 8:04 PM.

Details

Summary

In order to implement frontend caps on the size of logs shown, we need to subscribe to
stdout and stderr separately. This way, when stdout is large but stderr is not, we can cap stdout
subscriptions while still subscribing to stderr (and vice versa).

Is necessary for D1221

Test Plan

bk, viewed 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.Fri, Oct 4, 8:04 PM
prha updated this revision to Diff 5621.Fri, Oct 4, 8:20 PM

update schema

prha edited the summary of this revision. (Show Details)
prha added a reviewer: Restricted Project.
alangenfeld requested changes to this revision.Mon, Oct 7, 2:47 PM
alangenfeld added a subscriber: alangenfeld.
alangenfeld added inline comments.
js_modules/dagit/src/schema.graphql
1033

there are enums in GraphQL, we should use that here

This revision now requires changes to proceed.Mon, Oct 7, 2:47 PM
prha updated this revision to Diff 5681.Mon, Oct 7, 7:09 PM

use graphql enum

prha updated this revision to Diff 5682.Mon, Oct 7, 7:25 PM

use enum for ioType arg

alangenfeld accepted this revision.Mon, Oct 7, 8:24 PM
alangenfeld added inline comments.
js_modules/dagit/src/schema.graphql
40

should we expose COMPLETE?

python_modules/dagster-graphql/dagster_graphql/schema/runs.py
105–107

the asymmetry between query and subscriptions feels a bit off here. Regardless of whether we keep or change the schema, we should avoid reading the log files unless those fields are queried - we should move the read_logs_file call to the resolver on ComputeLogs

This revision is now accepted and ready to land.Mon, Oct 7, 8:24 PM
prha updated this revision to Diff 5706.Mon, Oct 7, 11:57 PM

comments from @alangenfeld, hiding COMPLETE as internal implementation detail, moving fetch to ComputeLog resolvers

prha added inline comments.Tue, Oct 8, 12:09 AM
python_modules/dagster-graphql/dagster_graphql/schema/runs.py
105–107

Yeah, agree it feels off. I initially removed this altogether... nothing consumes this except for tests at the moment.
I kept it in because I'm not sure how "correct" it is to always use subscriptions for the initial load.
I'll move the data fetching to the ComputeLogs resolver for now.