Page MenuHomePhabricator

Add error handling for get_or_create_run race condition
ClosedPublic

Authored by catherinewu on Apr 29 2020, 5:43 AM.

Details

Summary

Handle the possibility of concurrent RunStorage add_run() by trying to fetch the run again after DagsterRunAlreadyExists

https://github.com/dagster-io/dagster/issues/2412

Test Plan

todo

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

catherinewu created this revision.Apr 29 2020, 5:43 AM
catherinewu requested review of this revision.Apr 29 2020, 5:58 AM
catherinewu edited the summary of this revision. (Show Details)Fri, May 15, 4:49 AM
schrockn requested changes to this revision.Fri, May 15, 2:00 PM

Good stuff.

It would be good to test this codepath. Should be possible with come clever mocking

python_modules/dagster/dagster/core/instance/__init__.py
613–637

agreed.

I also think we should do some things to the naming to make this clearer (feel free to do this in follow on)

  1. Make it clear that this is the codepath for creating "MANAGED" runs. register_managed_run maybe?
  2. Eliminate the status argument.
  3. While we're at it, eliminate all default values for the arguments. It is super easy to forget one and they cause more harm than their potential convenience.
This revision now requires changes to proceed.Fri, May 15, 2:00 PM

add to test

schrockn accepted this revision.Tue, May 19, 1:00 PM

Thanks for wading through this. Feel to merge once tests pass

python_modules/libraries/dagster-airflow/dagster_airflow_tests/test_factory/test_pipeline_run.py
23–30 ↗(On Diff #14232)

great

This revision is now accepted and ready to land.Tue, May 19, 1:00 PM

use tempdir for test