Differential D1221

add client-side compute log size cap
ClosedPublic
Actions

Authored by prha on Fri, Oct 4, 9:44 PM.

Tags

None

Subscribers

sashank, natekupp

Details

Reviewers

Group Reviewers

Restricted Project

Commits

R1:c51387a521de: add client-side compute log size cap

Summary

Depends on D1219

Test Plan

Set client-side cap low, saw subscription connection close, warning sign

Diff Detail

Repository

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, 9:44 PM

Harbormaster failed remote builds in B4515: Diff 5630!Fri, Oct 4, 9:50 PM

tests

prha edited the summary of this revision. (Show Details)Fri, Oct 4, 10:15 PM

prha added a parent revision: D1219: Change compute logs graphql endpoint to query stdout/stderr separately.

prha mentioned this in D1219: Change compute logs graphql endpoint to query stdout/stderr separately.

prha added a reviewer: Restricted Project.

Harbormaster completed remote builds in B4522: Diff 5638.Fri, Oct 4, 10:18 PM

should we continue to stream logs but only retain/show the last X MB, similar to Buildkite?

sashank added a subscriber: sashank.Fri, Oct 4, 10:22 PM

sashank added inline comments.

js_modules/dagit/src/plan/ComputeLogContent.tsx
80–84	Just a very minor suggestion, but maybe change this to a link that downloads the log directly? So the user doesn't have to read this and then go find the download button everytime, and instead the action is inline with the call to action.

update based on comments

Harbormaster completed remote builds in B4524: Diff 5642.Sat, Oct 5, 12:54 AM

alangenfeld accepted this revision.Mon, Oct 7, 2:56 PM

This revision is now accepted and ready to land.Mon, Oct 7, 2:56 PM

prha mentioned this in R1:a9bae2261184: Change compute logs graphql endpoint to query stdout/stderr separately.Tue, Oct 8, 12:10 AM

Closed by commit R1:c51387a521de: add client-side compute log size cap (authored by prha). · Explain WhyTue, Oct 8, 12:15 AM

This revision was automatically updated to reflect the committed changes.

prha added a commit: R1:c51387a521de: add client-side compute log size cap.

Revision Contents
Changeset List

			Path	Packages
M			js_modules/dagit/src/__tests__/__snapshots__/App.test.tsx.snap (64 lines)
M			js_modules/dagit/src/__tests__/__snapshots__/ExecutionPlan.test.tsx.snap (20 lines)
M			js_modules/dagit/src/__tests__/__snapshots__/ExecutionPlanBox.test.tsx.snap (28 lines)
M			js_modules/dagit/src/plan/ComputeLogContent.tsx (123 lines)
M			js_modules/dagit/src/plan/ComputeLogModal.tsx (43 lines)

Diff 5710

js_modules/dagit/src/tests/snapshots/App.test.tsx.snap

Loading...

js_modules/dagit/src/tests/snapshots/ExecutionPlan.test.tsx.snap

Loading...

js_modules/dagit/src/tests/snapshots/ExecutionPlanBox.test.tsx.snap

Loading...

js_modules/dagit/src/plan/ComputeLogContent.tsx

Loading...

js_modules/dagit/src/plan/ComputeLogModal.tsx

Loading...