Page MenuHomePhabricator

[dagster] EventSink
ClosedPublic

Authored by alangenfeld on Aug 15 2019, 6:25 PM.

Details

Reviewers
schrockn
max
Group Reviewers
Restricted Project
Commits
R1:8911ca4db53d: [dagster] EventSink
Summary

Further iteration on the system to control communicating events that happen during execution.

Open to better names if anyone has them.

Test Plan

unit tests

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

alangenfeld created this revision.Aug 15 2019, 6:25 PM
alangenfeld edited the summary of this revision. (Show Details)Aug 15 2019, 6:40 PM
alangenfeld added a reviewer: Restricted Project.
alangenfeld updated this revision to Diff 3736.Aug 15 2019, 6:56 PM
alangenfeld edited the summary of this revision. (Show Details)

add on_pipeline_init and on_pipeline_teardown methods

Harbormaster failed remote builds in B2987: Diff 3735!
Harbormaster failed remote builds in B2988: Diff 3736!
alangenfeld planned changes to this revision.Aug 15 2019, 7:05 PM
alangenfeld added inline comments.
python_modules/dagster/dagster/core/events/event_sink.py
29–32

we can add stuff like on_log_message and on_dagster_event as they become useful

schrockn accepted this revision.Aug 15 2019, 9:01 PM
schrockn added subscribers: max, schrockn.

This is quite lovely. @max should take a look prior to merging but this looks great.

python_modules/dagster/dagster/core/events/sqlite_event_sink.py
133

questions:

  • Should this be part of the abc?
  • Can you document this to describe the semantics of this more exactly? Not exactly clear from reading the code.
This revision is now accepted and ready to land.Aug 15 2019, 9:01 PM
alangenfeld added inline comments.Aug 15 2019, 9:06 PM
python_modules/dagster/dagster/core/events/sqlite_event_sink.py
133

Should this be part of the abc?

I dont think so - this particular event sink is used to do cross process communication but others may just be writing to databases. We can move it later if thats not the case.

Can you document this to describe the semantics of this more exactly? Not exactly clear from reading the code.

good call, comments and check incoming

max accepted this revision.Aug 16 2019, 10:17 PM
This revision was automatically updated to reflect the committed changes.