get_stored_events and get_stored_runs
AbandonedPublic
Actions

Authored by • yuhan on Jun 10 2021, 8:58 AM.

Details

Reviewers

Summary

depends on D8213

Create StoredEventRecord to represent an event row stored in the event storage
Rename get_event_rows to get_stored_events and return List[StoredEventRecord]
To make RunStorage and EventLogStorage share consistent pattern, this diff also renames get_run_records to get_stored_runs and the returned representation from RunRecord to StoredEventRecord

will land it together with D8213 to avoid two rounds of internal repo changes

Test Plan

Repository

R1 dagster

Branch

arcpatch-D8213

Lint

Lint Passed

Unit

No Test Coverage

• yuhan requested review of this revision.Jun 10 2021, 9:22 AM

• yuhan retitled this revision from get_stored_events get_stored_runs to get_stored_events and get_stored_runs.Jun 10 2021, 9:03 PM

• yuhan added a child revision: Restricted Differential Revision.Jun 10 2021, 10:12 PM

python_modules/dagster/dagster/core/instance/__init__.py
990	curious what alex thinks but I actually prefer the old name - every run is already 'stored', i think records does a better job distinguishing?

Add some other reviewers so that hopefully this is the last rename diff

StoredPipelineRun StoredEventRecord (current diff)
PipelineRunRecord EventRecordRecord (not sure how to deal with event record here)
PersistedPipelineRun PersistedEventRecord
PipelineRunSnapshot EventRecordSnapshot
other ideas ?

python_modules/dagster/dagster/core/instance/__init__.py
990	the issue is `EventRecord` is already used so we need a new name if we want consistency between events and runs.
python_modules/dagster/dagster/core/storage/pipeline_run.py
399	i like this name

python_modules/dagster/dagster/core/instance/__init__.py
990	Could we use the serializer/deserializer logic to remap the legacy `_EventRecord` to a new class (e.g. `RunEvent`) and reuse `EventRecord` to be on parity with `RunRecord`).

python_modules/dagster/dagster/core/instance/__init__.py
990	ya i think thats possible if we prefer the `record` name enough to justify the complexity. It would be good to keep writing out to `EventRecord` too to avoid cross version breakage. I know we don't guarantee things but based on dagster-support we thrash a good number of people when we introduce cross version incompatibilities

python_modules/dagster/dagster/core/instance/__init__.py
990	I do like the `record` name. So it will be `get_run_records -> RunRecord` and `get_event_records -> EventRecord`. (`EventRecord` isn't a public api so it should be ok to use the name but slightly change the attributes of it.) I was making the easiest change for this RFC, but if we agree on the `record`, i'll go ahead and do the name remapping, i.e. legacy `EventRecord` -> `EventLogEntry` (or `EventEntry`?) - bc the method is called `get_logs`, maybe can use the name "log" in the returned type to make it more consistent. reuse `EventRecord` for the `get_event_records` - thank god that we didn't have `get_event_records`