Page MenuHomeElementl

get_stored_events and get_stored_runs
AbandonedPublic

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

Details

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

bk

Diff Detail

Repository
R1 dagster
Branch
arcpatch-D8213
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

yuhan requested review of this revision.Thu, Jun 10, 9:22 AM
yuhan retitled this revision from get_stored_events get_stored_runs to get_stored_events and get_stored_runs.Thu, Jun 10, 9:03 PM
yuhan added a child revision: Restricted Differential Revision.Thu, Jun 10, 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

wip: get_event_records and get_run_records

  • legacy EventRecord -> EventLogEntry
  • reuse EventRecord for the get_event_records
yuhan removed a child revision: Restricted Differential Revision.Thu, Jun 17, 1:17 AM