Add keyspace and make notifiers async #10
Conversation
uncleDecart
force-pushed
the
add-keyspace
branch
from
c91d3bc
to
851456d
Compare
|
Okay, I want to add benchmarks and load testing to see how this new version is going to behave, plus I want to do a small screen recording with terminals showing nkv client and server and work. You're right, it's about API, not language. So I moved stuff around, removing almost everyting related to rust under different docs and feel free to ask me any questions, this change was kind of not intuitive and took me a while to figure it out |
|
benchmark results are just awful, regression by 1000% is not okay, I guess there's something wrong with my trie implementation and problems with multithreading programming I did, investigating. Such a good tool though, this criterion.rs |
docs/KEYSPACE.md
Outdated
| # Notify Key Value (nkv) keyspace | ||
|
|
||
| Notify Key Value provides in-built keyspace functionality with `.` symbol as separator supporting wildcards. | ||
| What does that mean? Well, values are stored hierarchichly and each level of hierarchy is separated by `.` |
There was a problem hiding this comment.
I need a spell checker in helix (or vim) :DD
docs/DESIGN.md
Outdated
| @@ -0,0 +1,28 @@ | |||
| # How does `nkv` work exactly? | |||
|
|
|||
| `NotifyKeyValue` is a map of String Key and a value containing `PersistValue` and `Notiffier`. | |||
There was a problem hiding this comment.
Maybe Notifier (single f)?
There was a problem hiding this comment.
Does this mean server side? What are PersistValue and Notifier? Wouldn't someone coming here expect that it is just string key and value arbitrary bytes? How does this relate to the network access?
There was a problem hiding this comment.
fixed f and add another paragraph explaining it better (I hope :D)
| # How does `nkv` work exactly? | ||
|
|
||
| `NotifyKeyValue` is a map of String Key and a value containing `PersistValue` and `Notiffier`. | ||
| `PersistValue` is an object, which stores your state (some variable) on file system, so that |
There was a problem hiding this comment.
on file system
Is that inherent to it? Isn't that an implementation detail? From the client's perspective, isn't it, "I send it data, I get data"?
There was a problem hiding this comment.
It's a question of expectations: is nkv an API or do we also want to promise persist storage? I think we can separate that in a different PR (there's an open issue for it) and change docs to show components interaction and their promise (i.e. value is just an interface to store the value somewhere, be it persist, sending over network, writing to your flash drive, etc.) but that will require some renaming and extracting interfaces :)
|
Edit: I was comparing apples to oranges, meaning results from my laptop vs github runner, on same machine it shows significant perfomance degradation So apparently it's faster everywhere, except server, because in server benchmarks we wait to receive updated value from notification, and now we're doing it lazy way, meaning we have a separate thread to send notifications to all clients and we kick a notification channel to do so, that's why delays, it's really huge, like could be 10x slower, but I'm not sure how we can improve that. Let's talk tomorrow and see about that. Meanwhile I'll just touch up some other things... |
uncleDecart
force-pushed
the
add-keyspace
branch
5 times, most recently
from
0c85f5c
to
1b880c0
Compare
for more information why we need keyspace refer to #6 Signed-off-by: Pavel Abramov <[email protected]>
This is useful struct for O(n) for keyspace Signed-off-by: Pavel Abramov <[email protected]>
So now you can subscribe to a keyspace and if any changes occur all the parents will be notified. Also notifications are now fully asynchronous, so it won't block anyone to send updates
Signed-off-by: Pavel Abramov <[email protected]>
Signed-off-by: Pavel Abramov <[email protected]>
Signed-off-by: Pavel Abramov <[email protected]>
1. Now it is main_ci and pr_ci for each event that matters for us 2. During PR CI we run benchmars comparing performance of main to PR Signed-off-by: Pavel Abramov <[email protected]>
Signed-off-by: Pavel Abramov <[email protected]>
So that we don't have any performance regressions Signed-off-by: Pavel Abramov <[email protected]>
Signed-off-by: Pavel Abramov <[email protected]>
So when I have values in key1 and in key1.key2 when I get key1 I get value key1 not both Signed-off-by: Pavel Abramov <[email protected]>
Signed-off-by: Pavel Abramov <[email protected]>
Signed-off-by: Pavel Abramov <[email protected]>
Signed-off-by: Pavel Abramov <[email protected]>
uncleDecart
force-pushed
the
add-keyspace
branch
from
66c93c0
to
ac56acb
Compare
|
Fixed some document typos and added handler for client. So this is server build with debug symbols and two clients connecting to it, one is putting value, subscribing to updates, and another getting value and updating it. |
|
So now this PR also fixes #7 |
Signed-off-by: Pavel Abramov <[email protected]>
So that client can distinguish which key was deleted, created, updated Signed-off-by: Pavel Abramov <[email protected]>
|
Looking at the client times with TCP stack it makes me think that the problem could be in Benchmarking suite, real perfomance is 1ms per 1K of data compared to 72 ms in tests. I think we should merge this @deitch and then see to the benchmarking problem separately, I think these changes are enough to demo nkv to the team and gather feedback |
Still need proper documentation with design decisions and nkv limitations in place, once I'll add those I'll describe also the context of this PR and why notifications are now asynchronous. Will ping you once I'm done, Avi :)