I'd like to thank Marcelo for his work. Here's my review:
Does this library bring real benefit to C++ developers for real world use-case?
Definitely. C++ needs good networking facilities and Redis is very popular and useful. It also fits Boost as we have been incorporating a set of libraries so useful people can't ignore it. This should also make people less resistant to Boost because of the "monolith" myth. The library would help Boost have an even better collection of network libraries, and this ecosystem is part of why boost has many users nowadays. As more and more people are moving to C++17, they don't need `boost` as some kind of `std2::` anymore and all we would be left with are discussions about those Reddit complaints.
Do you have an application for this library?
No.
Does the API match with current best practices?
Although it doesn't require coroutines, its unapologetic use in the documentation is quite nice. This is not for all libraries but it's nice that users have more and more references of async code that looks clean. The pipelining interface is also quite nice and intuitive while still doing the hard work. Parsing directly into structures is also great. The internal use of `async_compose` is also clean, or at least as clean as it can be. If "co_await (conn.async_run() || conn.async_exec(req, adapt(resp)));" is a really common pattern, I found it weird that we didn't have a single operation for that. Other comments and examples in the docs helped clarify the issue but I have to admit I found it very confusing that early in the documentation. If they don't always have to go together, maybe we could start in the docs with examples that introduce them separately. Maybe we should have no support for async server pushes and requests in the first examples of the docs. If they really have to go together and really can't be another composed operation, maybe a diagram in the docs explaining the relationship between what these things are doing. If the examples in the docs are representative, `adapt` seems quite repetitive. Is `adapt` really needed? Can't the function or some function just accept `adapt`able types? `to_bulk` looks too general for ADL. Maybe use `tag_invoke`, specializations, or a longer name. `req.push_range` accepting iterators is kind of weird after the other overload does accept a range. At the same time, I understand this is only to differentiate from `req.push`, whose second argument can also be a range, so the common pattern of a single function name constraining the type is not a good solution in this case. So I don't know what's best because I'm bad with names, but it does look weird. Shouldn't `std::pmr::string& to` be `std::pmr::string& from`? And why not `string_view from`? I got confused with the requirements here even after checking the reference. The example with `dummy` didn't help much. Some API to create commands that are always correct seem useful since there seems to be little point in arbitrary command strings or commands without their parameters in the most common applications. I imagine real applications are limited to a small subset of commands. An enumeration to replace the command string doesn't look that good to me but I like Peter's idea of automatically generating the glue https://github.com/redis/redis/tree/4ffcec29af9dc1246540d94834540528576c68a4... . Otherwise, if they are manually maintained, they could exist only for the most common commands. Depending on the API, `push` could even become `raw_push` or something to disincentivize its usage. If new commands are specified but not implemented the user can always use "`raw_push`". Although I'm not sure it should be disincentivized in other RESP3 clients because I don't know what they are. Maybe *none* of that makes sense because there's a lot of conflict between redis and redis-like databases. I don't see how that could be at the moment because they would all be extensions. But then we would have a different problem because the documentation might be slightly misleading in representing what the library is intented to achieve, beginning with its name. What is the definition of a redis-like database, what does this ecosystem look like, what are the commonalities, conflicts, pros, cons, etc...? We had very similar discussions when Boost.MySql was proposed. And the pros and cons I hear are very similar. It feels like some kind of deja vu. I've been wondering if the `req.push("HELLO", 3);` couldn't be implicit somehow, since it's always there and resp3 only supports, well, resp 3. Probably not because we don't know if that `resp3::request` is the first request. But maybe we could think of something. In any case, it looks a lot like a TLS socket where its "hello" (big quotes here) is mandatory, but then there's the pipelining issue and the cost of maintaining and checking the "already_hello" (big quotes again) state. I also noticed we had some discussions about pipelining in Ruben's library. Given the direction Boost seems to be going, I expect that to come up more and more often. I have no idea what the best API would look like, but maybe you guys could discuss that a little. It might make sense to have some convention for this kind of thing for future reference. Or to come up with and explore some alternatives. If I understand Ruben's comments correctly, part of the problem here is communicating errors.
Is the documentation helpful and clear?
The template is nice and doesn't hurt my eye. It would be nice to have different pages instead of anchors to make it easier to navigate. But that might be a matter of taste. `aedis::resp3::request::config::coalesce` is mentioned before it's explained. Either it's important at this point or it isn't. "The request can contains" -> "contain" The `to_bulk` example has an unused variable. It's easier to have a simpler `my_struct` and show and example that really does the thing. redis-rs might be too slow for the plot but the number can still be mentioned in the text. It can also be represented with a truncated bar chart, which is not uncommon. I don't see the point of saying libuv is in the benchmark and then saying it's removed from the benchmark because of this and that. Just don't mention it. Same for the holes in the bar chart. Just don't put them there. Although the redis-rs could still be there as a truncated bar. They also exist in Latex. The hierarchy of the sections should have the echo server benchmark under "Comparisons". With time, the documentation could discuss the most relevant aspects of Redis directly without expecting the user to always refer to the Redis documentation. This helps explain the rationale for each decision. At some point, everyone will need to read the original for a real application. But initially, the docs could be at least in a way the user could get started without knowing much about Redis. For instance, the introduction kind of assumes the user knows what data structures Redis support. And the introductory example already uses an `HGETALL` command which is kind of "advanced" in comparison to simple strings. At this point, the user might think that the most common data structure, or only the data structure, etc, and give up trying the library. For instance, if `connection` MUST be discussed before the data structures, it would be more sensible to use only strings in this introductory example. Then it goes from "Connection" to "Server pushes" before discussing these types. Server pushes are not even included in the long Redis tutorial because it's considered advanced there. Between "Connection" and "Server pushes", we could have a least a small section with a table describing the supported data types and examples of equivalent C++ data structures that can hold these results by default. The official Redis tutorial is actually a good guide in terms of the order of topics for someone who doesn't know that much about Redis. One nice thing to try is to achieve something more goal-oriented, like other boost docs and the very Redis tutorial. This tends to be useful to users regardless of how much they know about Redis. The installation section could (should?) come at the beginning. I believe many of the issues raised in other reviews could be covered in the documentation and that would have avoided a lot of debate.
Did you try to use it? What problems or surprises did you encounter?
Yes. It worked fine with recent versions of the GCC, Clang, and MSVC. I would move everything in the `detail` namespace to files in `aedis/detail` to make the public API easier to read.
What is your evaluation of the implementation?
It's well-organized and reasonably documented when compared to other boost libraries. Some type requirements could be better explained in the reference.
Stating your knowledge of redis and asio would be helpful for me.
I've used Redis a few times in the past, I understand what it does, how it's no useful but I'm no expert. I understand the Asio primitives well. I'm not an expert on coroutines.
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
I recommend accepting the library. the reasons for your decision. I spent 1 1/2 days evaluating the library. Some decisions about the API led to discussion but the author has always had a reasonable rationale for them even when they are controversial. The library also uses Asio properly which, for better or worse, is not that easy to achieve. I mentioned reasons why I believe this kind of library is useful to the boost ecosystem. I'm not making the acceptance conditional, but I would like a commitment from the author to improve the most reasonable and critical points mentioned in other reviews by creating, tracking, and exploring issues. The repository also needs the boost boilerplate (j2, boost namespaces, etc...) but I assume that's implied unless I missed something. For now, I also see no reason not to honestly consider a "safe" API for at least a small subset of important and valid commands, or something more automatic and maintainable like Peter suggested. It seems like that could be doable with a python script. Thank you again for the submission, Em dom., 15 de jan. de 2023 às 12:17, Klemens Morgenstern via Boost < boost@lists.boost.org> escreveu:
The formal review of the aedis starts today (15th) and ends next week on Tuesday (24th).
The library was developed & submitted by Marcelo.
Aedis =================
Aedis is a Redis client library built on top of Boost.Asio that implements the latest version of the Redis communication protocol RESP3.
Some of its distinctive features are * A connection class that provides high-level functions to execute Redis commands and receive server pushes concurrently. * Automatic pipelining of commands. * Support for STL containers and serialization of user-defined data types. * Low-latency: Responses can be read in their final data-structure without creating copies.
Aedis links Github: https://github.com/mzimbres/aedis Homepage: https://mzimbres.github.io/aedis/ Git tag: v1.4.1 Tarball: https://github.com/mzimbres/aedis/archive/refs/tags/v1.4.1.tar.gz
Redis Links: Redis: https://redis.io/ RESP3: https://github.com/redis/redis-specifications/blob/master/protocol/RESP3.md Pipelines: https://redis.io/docs/manual/pipelining/ Redis commands: https://redis.io/commands/
Local Redis =========== Redis can be also easily installed on your local machine, for example, on Debian run "apt install redis". You might also want to use docker https://www.docker.com/blog/how-to-use-the-redis-docker-official-image/
Redis server for the Boost review ================================= We also have a public aedis server available for testing, please contact me or Marcelo for the details.
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.
The review is open to anyone who is prepared to put in the work of evaluating and reviewing the library. Prior experience in contributing to Boost reviews is not a requirement.
Some questions to consider:
- Does this library bring real benefit to C++ developers for real world use-case? - Do you have an application for this library? - Does the API match with current best practices? - Is the documentation helpful and clear? - Did you try to use it? What problems or surprises did you encounter? - What is your evaluation of the implementation?
Additionally, stating your knowledge of redis and asio would be helpful for me.
More information about the Boost Formal Review Process can be found at: http://www.boost.org/community/reviews.html
Reviews can also be submitted privately to me, and I will not disclose who sent them. I might however cite passages from those reviews in my final decision.
Thanks to Marcelo for providing us with a new library & thanks for all the reviews in advance.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
-- Alan Freitas https://alandefreitas.github.io/alandefreitas/ https://github.com/alandefreitas