-
Notifications
You must be signed in to change notification settings - Fork 628
Add a pubsub database implementation #4265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
21cda3f to
473a7cb
Compare
Zac-HD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High-level feedback: I'm really excited to have a high-performing option for this!
Small design change to support that: we should notify only on save events (inc. move), and pass the listener the changed k:v pair(s), rather than just a "something changed" notification. That way listeners never need to rescan the database!
|
Not on deletion also? We should definitely pass the k:v pairs (I realized this last night), but if we want consumers to be able to losslessly reconstruct the db state without rescanning then we also need to fire deletion events right?
|
|
Yeah, good point. Let's make it a stream of ("save"/"delete", key, value). Then our watcher can subscribe, scan once, and replay updates onto a cheap-to-query |
|
hm, problem: We could add a new e: probably better to do one |
|
idea: just have a |
|
that works wonderfully! One other snag: |
473a7cb to
434d2fe
Compare
|
This was an enormous pain to test, but on the bright side I am confident in the implementation now, and have picked up on a few minor implementation savings (move with src == dest in redis) along the way. I half expect these tests to be flaky though... |
434d2fe to
18e0ab1
Compare
9e50226 to
2d451c7
Compare
4cbdcd1 to
c6c2837
Compare
| ("delete", (b"k2", b"v3")), | ||
| ("save", (b"k1", b"v3")), | ||
| ] | ||
| elif sys.platform.startswith("win"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the linux details for move here are weird, but not really harmful, just redundant/inefficient. I suspect this is at the os/watchdog level but don't have a linux machine to test further. We use os.renames for db.move with a fallback to save/delete, which may explain some of the divergent behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github "codespaces" might be helpful to debug?
Zac-HD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, comments below are non-blocking 🙂
| ("delete", (b"k2", b"v3")), | ||
| ("save", (b"k1", b"v3")), | ||
| ] | ||
| elif sys.platform.startswith("win"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github "codespaces" might be helpful to debug?
c6c2837 to
7396cb1
Compare
TODO: write real docs for this? and figure out whether to put
watchdogintest.inortools.in?