Re: aedis review starts today

From: Alan de Freitas (alandefreitas_at_[hidden])
Date: 2023-01-24 02:52:02

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/src/commands
.
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.

I recommend accepting the library.

> Also please indicate the time & effort spent on the evaluation and give
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_at_[hidden]> 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>