Re: aedis review starts today

From: Marcelo Zimbres Silva (mzimbres_at_[hidden])
Date: 2023-01-21 17:16:14

On Sat, 21 Jan 2023 at 16:57, Ruben Perez <rubenperez038_at_[hidden]> wrote:
>
> Final set of questions, I hope.
>
> 1. If you write something like
>
> aedis::request req;
> std::tuple<std::optional<std::string>> res;
> req.push("LPOP", "something_not_a_list"); // will cause an error
> co_await conn.async_exec(req, adapt(res));
>
> You seem to get an error code reported. The server does send an
> error response with diagnostics, but I haven't found a way to access
> that message.  Is there any way?

One way is to change the response type to
`std::vector<resp3:node<String>>` so it can store both success and
error replies (without discarding the error message), namely

* On error vec.front().value will contain the diagnostic error message
sent by the server. And vec.front().data_type will be
resp3::type::simple_error.
* On success vec.front().value will contain the expected value and
vec.front().data_type will be resp3::type::blob_string.

This is a bit cumbersome but works. There are plans to simplify this,
follow this issue for more
https://github.com/mzimbres/aedis/issues/40.

I know Boost.MySql invented error_info to deal with this problem. I
did not adopt it because

* Deviates the completion signature from well established practice of
having error_code as first parameter f(erro_code ec, ...) or
* Requires additional parameters to the completion e.g. f(error_code
ec, error_info ei, ...)
* Raises question about who allocates the string.
* The error information is also only useful for logging and the user
can't react to it so why not log it right away.

> 2. I've modified cpp20_subscriber to add another task that sends commands
>     with async_exec while reading subscription replies. If any of the commands
>     issued with async_exec contain any errors (like the LPOP above), the program
>     crashes with a "conn->cmds_ != 0" assertion.

That is probably a bug in the demultiplexing of events (the most
complex thing in Aedis). You shouldn't be able to cause any crash.
Tracking it here: https://github.com/mzimbres/aedis/issues/50 and
thanks for reporting.

> 3. I've seen other Redis (async) libraries implementing connection pools.
> Unless I'm mistaken, it seems likely that some of the user base will
> eventually require this feature. I see some overlap between the queuing
> system you have in place in connection and the responsibilities of the
> connection pool (I'd usually implement this kind of queueing at the
> connection pool level).  What's your view of this?

All libraries I am aware of don't provide event demultiplexing and
reconnectable connections, therefore the connection cannot be shared
among e.g. individual http or websocket sessions in a server. To
circumvent this problem connection pools must be implemented. They add
an overhead though

* Increased number of connections on the Redis server.
* Loses the ability to implement eficient command pipelines, which
will increase the number of syscalls: instead of writing all requests
once (one syscall), each connection will write its own request (n
syscalls). The same problem is also valid for reading.
* Increased usage of resources: 1 vs n connection objects.
* Increased overall complexity: again 1 vs n connection objects.

So I would say Aedis doesn't need connection pools.

>     a. Is connection pooling planned in the future for Aedis? Or is it
>     considered the responsibility of a higher-level component?

Ditto. Aedis doesn't need them for the reasons stated above.

>     b. In the latter case, would that component have to make use of the low-level API?

If somebody wants to implement a connection pool it could use the
connection object directly I would say.

Regards,
Marcelo