dnsdist: add redis KVStore implementation#17441
Conversation
Signed-off-by: Ensar Sarajčić <dev@ensarsarajcic.com>
|
NOTE: this is just the first PR in sequence, if this is accepted. The actual implementation has a lot more commands and also caching support (all available in Quad9 fork: https://github.com/Quad9DNS/pdns-redis/tree/feature/redis-kvstore -- this is actually related to #17387 too, since it is included and mixed in https://github.com/Quad9DNS/pdns-redis/tree/redis-cuckoo-mmdb with this, but it was much easier to extract). I wanted to get review on the general approach taken with Redis, which maybe has too many classes (each reply, command, lookup action and some variants of clients get a class), before introducing further PRs with more complex parts of this (like caching). This is also probably missing some important parts, since I just yanked it out of the implementation in the fork, but I can fix these after review. |
94edd48 to
d4b483c
Compare
Signed-off-by: Ensar Sarajčić <dev@ensarsarajcic.com>
Signed-off-by: Ensar Sarajčić <dev@ensarsarajcic.com>
rgacogne
left a comment
There was a problem hiding this comment.
Thanks! On principle I don't see any reason not to add this functionality, it would be nice to be able to query redis without going to Lua, for example.
I skimmed over the code and I don't really see the point of the pipeline mechanism, but it's probably because I'm not very familiar with redis. Would you mind explaining a bit more what the use-case is?
The idea is to batch commands in a single communication to redis, improving throughput. I believe for correct usage the user would have to tweak and play with the pipelining interval. When only using Redis from Lua, I think pipelining is useless, because it will block until the response arrives anyways and since Lua uses a single thread, no other request will be made in the meantime, but I think it can increase throughput when used from KVS, since it can then be used by multiple threads. |
How does that work? Isn't the KVS interface just waiting for the response as well? I don't see any async implementation that would suspend the query then resume it when the response comes up. |
|
|
||
| std::string uniqueId = "url=" + redisClient->getUrl().to_string() + ",action=" + lookupAction.value_or("GET") + ",data-name=" + dataName.value_or("") + ","; | ||
| std::string labels = "redis-server=" + redisClient->getUrl().host + ":" + std::to_string(redisClient->getUrl().port) + ",redis-action=" + lookupAction.value_or("GET") + ",data-name=" + dataName.value_or(""); | ||
| std::shared_ptr<RedisStats> stats = std::make_shared<RedisStats>(labels); |
There was a problem hiding this comment.
So the idea is to share metrics between all RedisKVStore instances using the same parameters, right? What's the use case for defining several of these?
There was a problem hiding this comment.
Hmm I wanted to prevent that case, to prevent confusion about the metrics. I thought the runtime error I threw below would prevent duplicate instances with the exact same params.
There is a use case for different parameters of course and that should be supported.
My assumption was that KVS can be used from multiple threads, so in that case, there might be multiple queries lined up and sent in a single communication - all of them will block until these responses come though. Maybe this is not such a good idea 😄 |
Signed-off-by: Ensar Sarajčić <dev@ensarsarajcic.com>
We might have to do a quick benchmark in any case, because it feels like we are adding latency rather than reducing it, but it's hard to know without some numbers :) |
Agreed. I will try to set something up to check this. |
On the other hand, this probably makes more sense if we know we want to send multiple commands at once, but all lookup actions are based on just a single command, so maybe this is just a needless complication. I will remove it for now and if I figure out a case when it creates a noticeable performance gain, I will add it in separately. |
Signed-off-by: Ensar Sarajčić <dev@ensarsarajcic.com>
Short description
Adds support for communicating with Redis, as well as a
RedisKVStoreimplementation, backed by a redis client (which can be used independently in Lua).Checklist
I have: