Page MenuHomePhabricator

[sqlite event log] writer thread
AbandonedPublic

Authored by alangenfeld on Sep 19 2019, 6:19 PM.

Details

Reviewers
max
schrockn
Group Reviewers
Restricted Project
Summary

We don't want to block on event storage so this adds a very basic writer thread. We can improve / switch to multiproc in the future.

Test Plan

buildkite

Diff Detail

Repository
R1 dagster
Branch
writer (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

alangenfeld created this revision.Sep 19 2019, 6:19 PM
alangenfeld edited the summary of this revision. (Show Details)Sep 19 2019, 6:31 PM
alangenfeld added a reviewer: Restricted Project.
alangenfeld planned changes to this revision.Sep 19 2019, 8:22 PM
alangenfeld updated this revision to Diff 4884.Sep 19 2019, 9:10 PM
alangenfeld edited the summary of this revision. (Show Details)

fire and forget thread instead of spinning consumer

max added a subscriber: max.Sep 19 2019, 9:24 PM
max added inline comments.
python_modules/dagster/dagster/core/storage/event_log.py
219

is there a danger of this blocking during GC?

236

won't this immediately either fail the queue.empty() test and exit, before the event is put in the queue? i think only the thread at l. 185 will ever actually execute anything

max accepted this revision.Sep 19 2019, 9:26 PM
This revision is now accepted and ready to land.Sep 19 2019, 9:26 PM
max added a comment.Sep 19 2019, 9:27 PM

Add a comment to explain the function of the first thread.

alangenfeld updated this revision to Diff 4904.Sep 20 2019, 4:33 PM

comments + fixes

alangenfeld updated this revision to Diff 4907.Sep 20 2019, 5:01 PM

another approach

alangenfeld updated this revision to Diff 4911.Sep 20 2019, 6:07 PM

once more with gusto

alangenfeld requested review of this revision.Sep 20 2019, 6:13 PM

this changed quite a bit would love another pass

alangenfeld updated this revision to Diff 4914.Sep 20 2019, 6:52 PM

py2 workaround

alangenfeld added inline comments.Sep 20 2019, 6:54 PM
python_modules/dagster/dagster/core/storage/event_log.py
167–169

new - resolved a race condition based on the logic at [1]

248

[1]

270–273

new - block reads on writes

max added inline comments.Mon, Sep 23, 6:23 PM
python_modules/dagster/dagster/core/storage/event_log.py
273

per our discussion in person, this feels too risky -- we may be throwing away the benefits of being async to make the tests easier to run as currently written

alangenfeld updated this revision to Diff 5006.Thu, Sep 26, 4:10 PM

rebase & dont block

alangenfeld updated this revision to Diff 5008.Thu, Sep 26, 4:28 PM

fix tests - but this still doesnt handle errors in a compelling way and im starting to believe we dont want to land this

schrockn requested changes to this revision.Thu, Sep 26, 4:42 PM
schrockn added a subscriber: schrockn.

bouncing from queue

This revision now requires changes to proceed.Thu, Sep 26, 4:42 PM
alangenfeld abandoned this revision.Thu, Oct 3, 7:29 PM

for now at least