Page MenuHomePhabricator

Add structured display for intermediate store events
ClosedPublic

Authored by max on Jul 23 2019, 12:34 AM.

Details

Summary

Add structured events for object store operations.

Will move message generation & cut metadata entries server-side in a follow-on diff (with other message types).

Test Plan

Unit

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

max updated this revision to Diff 3090.Jul 23 2019, 12:34 AM
max created this revision.

Nit

max updated this revision to Diff 3099.Jul 23 2019, 6:41 PM

Test fixes

max updated this revision to Diff 3102.Jul 23 2019, 6:47 PM

Test fixes

max added reviewers: schrockn, Restricted Project.Jul 23 2019, 7:08 PM
max edited the summary of this revision. (Show Details)
max edited the summary of this revision. (Show Details)Jul 23 2019, 7:22 PM
max added a comment.Jul 24 2019, 5:47 PM

Following diff will normalize metadata and message construction with all of our other events.

max updated this revision to Diff 3117.Jul 24 2019, 5:59 PM

Rebase

natekupp added inline comments.
python_modules/dagster/dagster/core/storage/object_store.py
113

use the enum value ObjectStoreOperationType.GET_OBJECT instead of strings? same for other places where we use these strings

Python side looks reasonable to me w/ one comment, will let others review the front-end side

sashank added inline comments.
python_modules/dagster-graphql/dagster_graphql/implementation/pipeline_execution_manager.py
113 ↗(On Diff #3117)

Do we want this under event_specific_data, or directly on DagsterEvent? In https://dagster.phacility.com/D682, we have it stored directly on DagsterEvent.

sashank added inline comments.Jul 24 2019, 7:47 PM
python_modules/dagster/dagster/core/events/__init__.py
395

Would it be possible to store this as a EventMetadataEntry? Maybe as JsonMetadataEntryData. Then we don't need to use event_specific_data and have the new ObjectStoreOperationResultData type.

max updated this revision to Diff 3127.Jul 24 2019, 8:10 PM

Rebase and revise

max added inline comments.Jul 24 2019, 8:12 PM
python_modules/dagster/dagster/core/events/__init__.py
395

yes, I'd like to do this in the following diff where we normalize everything to use EventMetadataEntries

My only real concern with this diff is that it seems a little "thrashy" if we are just instead going to use a generic renderer client-side.

Once we render this generically this will have to be substantially rewritten.

By my count:

  1. No more custom client-side renderer: ObjectStoreOperationEvent will be gone
  2. Then server-side rendering of the description string
  3. Then using EventMetadataEntry.

Totally our discretion as to the right ordering.

I also might not be seeing something.

js_modules/dagit/src/runs/LogsRow.tsx
299

eliminate

314

so this is temporary (e.g. the entire ObjectStoreOperationEvent) is temporary until we do the fully generic rendering component?

max updated this revision to Diff 3128.Jul 24 2019, 8:18 PM

Undebug

max updated this revision to Diff 3131.Jul 24 2019, 9:52 PM

Re rebase

max updated this revision to Diff 3132.Jul 24 2019, 9:56 PM

Undebug

Harbormaster failed remote builds in B2519: Diff 3132!
max updated this revision to Diff 3138.Jul 24 2019, 10:22 PM

Regenerate

sashank accepted this revision.Jul 24 2019, 10:48 PM
This revision is now accepted and ready to land.Jul 24 2019, 10:48 PM
max updated this revision to Diff 3152.Jul 24 2019, 11:32 PM

Rebase & rework