Page MenuHomePhabricator

[sqlite event log] writer thread

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


Group Reviewers
Restricted Project

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


Diff Detail

R1 dagster
writer (branched from master)
Lint OK
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.

is there a danger of this blocking during GC?


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

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




new - block reads on writes

max added inline comments.Sep 23 2019, 6:23 PM

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.Sep 26 2019, 4:10 PM

rebase & dont block

alangenfeld updated this revision to Diff 5008.Sep 26 2019, 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.Sep 26 2019, 4:42 PM
schrockn added a subscriber: schrockn.

bouncing from queue

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

for now at least