Page MenuHomePhabricator

Add error handling for get_or_create_run race condition

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



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

Test Plan


Diff Detail

R1 dagster
Automatic diff as part of commit; lint not applicable.
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



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

23–30 ↗(On Diff #14232)


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

use tempdir for test