On Sun, 15 Jan 2023 at 16:17, Klemens Morgenstern via Boost <boost@lists.boost.org> wrote:
The formal review of the aedis starts today (15th) and ends next week on Tuesday (24th).
Hi all, This is my review of the proposed Boost.Aedis. First of all, thank you very much Marcelo for submitting the library, and for your quick responses to my (not few) questions. Thanks also Klemens for acting as review manager.
Does this library bring real benefit to C++ developers for real world use-case?
I strongly believe so. It integrates in the new Asio-based ecosystem we are creating, together with Beast and MySQL. Redis is popular. I buy the vision.
Do you have an application for this library?
I don't. I'm currently just maintaining Boost.MySQL, and there's little room for it in the task. But I've had it in the past - a company in the IoT world that was using Redis as a message-passing system. I would have liked having Aedis back then.
Does the API match with current best practices?
In terms of Asio primitives, yes, it does. It complies to the universal async model, which is what I'd expect. I've also liked how you implemented rebind_executor for SSL streams to make default completion tokens easier - which is something I've recently done in MySQL myself, too. This library introduces something unusual to Asio-based components, which is the command queue. I see advantages on having it (as it provides out-of-the-box efficiency), but also some downsides (it is opinionated, not allowing for strategies like connection pooling). I don't think this is necessarily a bad thing - field experience will give us insights. I do think this makes good docs even more essential. I like how adapt() works. Parsing into compile-time structures is always good. However, as a Redis novice I am, I've had a hard time debugging things when I got a mismatch between the type passed to adapt() and what the command expects. The error happens at parse time, which I think is pretty dangerous, as it's very easy to get things overlooked. As an example of this, the library's own example cpp20_containers, in the hgetall function, uses the type std::map<std::string, std::string> for the hgetall response, which will be okay unless the key is not set, in which case you will get an error. I think this has an enormous potential for causing hidden bugs (and even vulnerabilities). I've also noticed that if you mismatch the sizes of the pipeline request and response, you get an assertion (so UB). I initially went with a lot of (false) confidence, as adapt() changed my mind to "this is type safe, you're not writing C, the compiler will know if you mess it". It's true that if you think it twice, the compiler can't know, but this false confidence is something that may happen to more people. So I think this should somehow be addressed. Exposing just request::push() seems terse. I think some methods for the most common commands would help a lot. Something like request::push_set(string_view key, string_view value). Or maybe request::push(set_cmd(string_view key, string_view value)). Maybe doing that could help address the problem above. I still think a general request::push() should be exposed for the less common commands. The way of handling network errors is very elegant, and it shows the author's effort on it. The reconnect loop is clean. Congratulations. I think error handling at the command level is flawed, though. If a command fails, the entire pipeline operation fails, and no easy access to the diagnostics is provided. I've also seen that Redis will execute all commands in a pipeline even if an error is encountered in one of them. So, in pipeline [C1, C2, C3], if C2 errors, C3 is still executed. The API should provide a way to access C2's error and C3's result. The proposed solution in https://github.com/mzimbres/aedis/issues/40 seems insufficient to me. I'd give Boost.Leaf (or a similar solution) a thought. I don't know if making the connection hang when a push is received and there is no async_receive active is safe. I don't know enough of edge cases to know, but it causes me to raise an eyebrow as user, at least.
Is the documentation helpful and clear?
It is far away from the level I'd ask for a Boost library. Please remember that this is one of the criticisms Boost gets at all times, so we should pay special attention to it. What I think it can be improved: * It needs a proper discussion section, as other libraries do. Explaining goals, design decissions and usage patterns. I'd structure this as: * A proper tutorial. You may assume familiarity with Redis to the point "I know what HGETALL does" (although I would start with a SET/GET). But HELLO and QUIT are not obvious. I'd explain that this 3 is the protocol version, what happens if you don't send a HELLO, whether QUIT is handled specially or just like any other command, etc. Do not place an opaque function "connect" that is not part of the public API in a tutorial. The code should run "as-is". Provide also error handling code. * A design discussion, where you present your design choices with examples and snippets. I like how Boost.URL does it. You've implemented a pretty opinionated queueing system, so I'd draw a diagram of how that works, with the different tasks and how they relate to each other. Also state what benefits does it bring. It would be great if a user could figure out most of the questions I've asked you throughout the review by reading that document. I would include some of the code you currently have in examples as snippets, like the ones in containers, transactions, subscriptions, and so on. A section on error handling would be nice, too. Some docs on request::config flags and when to use each would be great, too. * An example section, where you provide the full listing of all examples. I'm currently using quickbook and its import facility to make this happen, and I'm pretty happy. * A proper reference.
Did you try to use it? What problems or surprises did you encounter?
* Doesn't build with clang15 and libc++ under Ubuntu 22.04, as <memory_resource> appears to not be stable yet (as of libc++ 15). * Builds and runs fine with gcc11 and stdlibc++ under Ubuntu 22.04. I was a little bit let down by finding what seems to be a bug in the queueing system by just playing with the subscription example. I see that all tests covering the connection class (and its underlying queue system) are integrated. This leads to the impression that edge and error conditions haven't been tested as I'd have expected.
What is your evaluation of the implementation?
I think it is good. As others have already looked into it, I just read enough to figure out pieces I was missing to understand the system under review, and won't comment it here.
Additionally, stating your knowledge of redis and asio would be helpful for me.
I have written a similar client library using Asio (Boost.MySQL). I have a shallow knowledge of Redis. I've read the documentation and played with it these days to get a refresher, but I'm far from an expert. I have invested around 2 days in playing with the library and writing this review.
Please explicitly state that you either *accept* or *reject* the inclusion of this library into boost. Also please indicate the time & effort spent on the evaluation and give the reasons for your decision.
I vote to REJECT the library in its current state. I think we need to make sure proper tests, documentation and error handling strategies are in place before proceeding. I think the library has gone in the right direction overall since I last looked at it. I would encourage Marcelo to keep working on it and resubmit it soon. I don't think a full review will be required then, but a mini-review should be enough. Some minor things: * resp3::node< String >: "String: A std::string-like type" - type requirements like this should be properly documented - what does that mean? Concept checking here would be great. * Would be great to provide concept checking for adapt() "The types T1, T2, etc can be any STL container, any integer type and std::string." can it not be aedis::ignore, too? * request::push and request::push_range: the reference contains no info on type requirements. * I would consider renaming the library to Boost.Redis, as it would make purpose clearer. * async_exec reference: "Multiple concurrent calls to this function will be automatically queued by the implementation." this can lead the user think the connection class is thread-safe, which is not. * As a user I wouldn't know if I can really use the low-level API or not. * #include <boost/asio.hpp> in examples makes the compiler cry ;) * The CMakeLists aren't compatible with the Boost superproject. * B2 support should be added at some point. * I think this library shows that Klemen's Asem should be part of Boost. Thanks again Marcelo for taking your time and effort. Regards, Ruben. On Sat, 21 Jan 2023 at 19:40, Marcelo Zimbres Silva <mzimbres@gmail.com> wrote:
On Sat, 21 Jan 2023 at 19:04, Jakob Lövhall <lovhall@protonmail.com> wrote:
making the error text available is not only logging. it can be passed on to a user in interactive sessions giving the user (human) ability to act on it right away. I wouldn't call that "only useful for logging" even though I guess it is logging in some sense.
Good point. I hadn't thought about interactive sessions.
Regards, Marcelo