Page MenuHomePhabricator

Improve event log error handling
ClosedPublic

Authored by max on Sep 17 2019, 9:22 PM.

Details

Reviewers
alangenfeld
schrockn
Group Reviewers
Restricted Project
Commits
R1:39e396cb6bf0: Improve event log error handling
Summary

Preparatory to frontend support for handling EventLogCorruptForRun, EventLogNotFoundForRun

Test Plan

Unit

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

max created this revision.Sep 17 2019, 9:22 PM
alangenfeld added inline comments.Sep 17 2019, 9:29 PM
python_modules/dagster/dagster/core/storage/event_log.py
29

maybe Invalid instead? I would interpret corrupt as a subset of invalid

101–104

if we want to enforce this behavior (which i fled from due to volume of test breaks) I think its better to just stop using defaultdict instead of adding a set

150

[2]

195

[1]

204

cast necessary given [1]

also is the problem here auto indexing starts at 1? given this sql i thought it was fine [2]

python_modules/dagster/dagster_tests/core_tests/storage_tests/test_event_log.py
18–144

yaytests

max updated this revision to Diff 4815.Sep 17 2019, 11:18 PM

has_run

max updated this revision to Diff 4816.Sep 17 2019, 11:27 PM

Black

max added inline comments.Sep 17 2019, 11:37 PM
python_modules/dagster/dagster/core/storage/event_log.py
101–104

eh, then we need to new up the semaphore, etc.

101–104

see D1051 -- it may make more sense to just remove this behavior (it isn't used anywhere)

alangenfeld accepted this revision.Sep 18 2019, 6:20 PM

proceed

python_modules/dagster/dagster/core/storage/event_log.py
101–104

depending on whether or not D1051 works I still think regular dict (and getting rid of locks entirely) would be a bit cleaner

This revision is now accepted and ready to land.Sep 18 2019, 6:20 PM