Re: aedis review starts today

From: Sam Hartsfield (samh.boost.tak6q_at_[hidden])
Date: 2023-01-24 14:52:21

Hello everyone,

This is my review of the Aedis library. This is my first attempt at a
Boost review,
so I hope that it is helpful. Also, I'm attempting to send this from Gmail, so I
apologize if the formatting doesn't come out right.

> Additionally, stating your knowledge of redis and asio would be helpful for me.

I am using Redis in a project, so I am familiar with it, though nothing
too advanced. I have looked at the Asio documentation before, but this is my
first time actually trying it out (I've been meaning to).

> Does this library bring real benefit to C++ developers for real world
> use-case?

Yes, absolutely, I think this is a great addition to the Asio ecosystem.

> Do you have an application for this library?

Yes, I'd like to replace the hiredis code in the aforementioned project
with something C++ with asynchronous support, but none of the existing
options seemed great, and I want to try to move to using Asio in the future,
so this library seems like a perfect fit.

> Does the API match with current best practices?

I'm not that familiar with Asio best practices.
I don't have a problem with the raw strings for this API; a more
strongly-typed API could be added on top later.

> Is the documentation helpful and clear?

Overall I thought it was a bit sparse.
I think the other reviews have made some great suggestions, in particular
Ruben Perez's comments on what could be added, such as a discussion
of design decisions.

I was initially put off by the lack of C++17 examples (since I'm not using it
yet for work), but thinking about it with a long-term view I think the C++20
style is a good thing.
However, I thought the use of the "common" files in the examples was
less than helpful; I think it would be easier to follow if the examples were
each complete (maybe some "common" code could be added to the library?).

Is it important to send "HELLO"? I don't see that explained.

In the Redis documentation for "QUIT", it says "Clients should not use this
command", so maybe you should explain why it is used.

It was not immediately obvious why a `tuple` is used for the response,
so maybe some explanation of that is warranted the first time it is shown,
or at least a reference to the "Responses" section (e.g. "response type will
be explained later in the 'Responses' section"). I'd also like to see an
explanation of why the `adapt` function call is required. Why can't I just
pass the response type?

Most of the examples show multiple commands, and it's great that it can
handle that, but it seems like a common case would be to execute a
single command and get a response. It seems like there should be some
shortcut included in the library, for example:

    std::optional<std::string> reply;
    co_await conn.async_exec(resp3::request("GET", "FOO"), adapt(reply));

> Did you try to use it? What problems or surprises did you encounter?

Yes, I played with the examples. I mostly tried using it in the C++17
style.

I had errors compiling the cpp17_intro.cpp example on GCC 10
(gcc-toolset-10 on RHEL8),so I had to switch to GCC 12 ("no matching function
call to from_chars...").

I initially tried to use the library  via CMake but Boost::asio was not a valid
target for me.

The first time I tried to modify one of the examples to do something
different, I got a SIGSEGV with no obvious indication of what went wrong.
It was an object lifetime issue - obvious in retrospect that the request
and reply objects need to live past the async_exec call, but I guess the
lambdas make it easy to forget how things are actually flowing. More
an Asio/C++ issue than Aedis.

This may also be due to my unfamiliarity with Asio, but when I tried the
"use_future" completion token, get() never returned:

    resp3::request req;
    req.push("QUIT");
    auto f = conn.async_exec(req, adapt(), net::use_future);
    std::cout << "QUIT: " << f.get() << std::endl;

> What is your evaluation of the implementation?

I didn't look very deeply into the implementation.

> 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 think the explanation from Ruben's review seems reasonable, so I will
also say REJECT in the current state, but I very much would like this library
to be included in Boost in the future. It sounds like the error handling may
need some more consideration, and I'd like to see the documentation
expanded.
I would also like to see the name changed to Boost.Redis if accepted,
to make it easier to identify and find.

I spent around 5-6 hours over a few days evaluating the library.

Thanks very much to Marcelo for working on this library!

- Sam Hartsfield