-
Notifications
You must be signed in to change notification settings - Fork 693
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
Add GETPXT, MGETPXT (Get with millisecond expiration) commands #1455
base: unstable
Are you sure you want to change the base?
Conversation
Returns null if the value not found or expired. Returns an array of length 2 as [<string key value>, <integer expiration>]. If expiration is not set on the key, expiration returned is -1. Added tests to cover Signed-off-by: Arcadiy Ivanov <arcadiy@ivanov.biz>
Is your application bottlenecked on throughput? The use case generally makes sense to me, but I would rather avoiding adding a command if we can optimize the existing codepath. I will say that fetching the expire time is getting a lot more efficient with the new cache efficient dictionary we are releasing with 8.1 (hopefully), so maybe it's not as critical. Another note, there is some overlap in naming with |
Two commands, apart from the overhead of processing two commands and potentially non-atomic nature of the retrieval (I'm not sure here as far as architecture - are pipelined commands atomic?), will end up calling I am not married to a command name and considered adding an option to the GET command, although this being the most frequently used command it would introduce parsing overhead (however small). Looking for further guidance. |
Pipeline commands are not atomic, but you can send them in a multi-exec which does make them a transaction.
Understood. However, on modern hardware the bottleneck is mostly on DRAM latency if you have a miss in the CPU cache, so practically the double miss won't matter much. Give me one sec, I'll come up with a procedure you can use to quickly test the performance. |
Yep, already using pipeline'd MULTI/GET/PEXPIRETIME/EXEC. The EXEC is marked "slow" in documentation. The above scheme, however, is entirely avoided if an atomic command is used 😄 |
EXEC itself is not slow. It's basically a no-op. MULTI-EXEC works in a very simple way. The commands are queued up and when EXEC is called, all the commands are executed together and this makes it atomic. So, you could say EXEC is slow, because it executes all the commands that have been queued up. If you have actually slow commands in the transaction, they will make EXEC slow. The "slow" flag in the documentation is mainly confusing and I wish we could just delete this in the documentation. Currently, every command that isn't explicitly marked as "fast" are marked as "slow".
I don't get this. MULTI/GET/PEXPIRETIME/EXEC is atomic, just like your proposed GETPXT. No need for WATCH, or am I missing something? |
Ah, nevermind, I read into the documentation - WATCH is only needed when used before the transaction is started with MULTI. Withdrawn. |
Remove the "overload" as non-precision timestamp isn't useful in the intended context Signed-off-by: Arcadiy Ivanov <arcadiy@ivanov.biz>
Since MULTI-EXEC can do this atomically, we will most likely not accept this new command without another good reason, something that MULTI-EXEC doesn't solve. Is the performance of MULTI-EXEC not good enough? |
@zuiderkwast well, there are atomic sets (
I will conduct the formal performance tests but given that a MULTI-EXEC will result in more parsing, more wire traffic and two lookups instead of one, I doubt the performance will be identical. Our goal is to fit as much performance as possible into the smallest machine possible to save costs. |
@arcivanov Thanks, so I can summarize you points like this:
In the past when there was a BDFL governance, commands got added based on personal taste. In the last years, we have been more restrictive in adding new commands, especially when the same effect can already be achieved. There's no absolute rule about this though. I'm looking forward to see your performance results. To me, it seems like the most convincing reason to add this. My guess is that a transaction is slower, but not many times slower. |
Are you querying multiple keys at the same time? If yes, then there's an alternative to consider. MGET can be used to get multiple keys. If we can allow PEXPIRETIME to return the timestamp for multiple keys, then you could fetch this info for multiple keys like
|
These are very preliminary and I'll push the code once I clean it up a little bit but here is the data: Average latency for Again, this is very preliminary and I need to exclude a few potential sources of error in the bench (namely
|
Signed-off-by: Arcadiy Ivanov <arcadiy@ivanov.biz>
I have pushed the changes to the benchmark and here are the results done over the 1GbE network of Framework 16 client over LAN with
|
Distributed cache synchronization is an eternal problem. Luckily, currently (2024), cloud deployed services provide high quality time synchronization for deployed resources, allowing precision timing coordination with sub-millisecond precision. This command allows to take advantage of this fact to implement simple distributed cache synchronization.
Using absolute Unix millisecond timestamps for synchronization allows machine-local caches that feed from a shared Valkey cache in the cluster to expire the local entries within 1ms of each other, assuming the sub-ms clock synchronization, providing no-cost distributed cache synchronization.
Unfortunately, Valkey does not have commands that allow to conveniently (and atomically!) retrieve a value and the accompanying expiration timestamp in a single command. The workaround exists allowing to use GET followed by PEXPIRETIME, pipelined. This naturally involves additional processing including checking for the key's existence twice.
This patch introduces a new command GETPXT (GET + PeXpireTime), which behaves as documented below and elsewhere in the commit.
Returns
null
if the value not found or expired.Returns an array of length 2 as
[<string key value>, <integer expiration>]
. If expiration is not set on the key, expiration returned is-1
.Added tests to cover