Fwd: Aedis Review

From: Klemens Morgenstern (klemensdavidmorgenstern_at_[hidden])
Date: 2023-01-24 05:24:13

This review was sent directly to me.

---------- Forwarded message ---------
From: Robert A.H. Leahy <rleahy_at_[hidden]>
Date: Tue, Jan 24, 2023 at 1:04 PM
Subject: Aedis Review
To: klemensdavidmorgenstern_at_[hidden] <klemensdavidmorgenstern_at_[hidden]>

Sending directly as I'm not on the Boost mailing list.

*Knowledge of Redis*

No more than can be gathered by Wikipedia.

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

Yes. Having an idiomatic (i.e. Asio-like) way to interact with something in
C++ brings real benefit.

*Do you have an application for this library?*

Not immediately but Redis is in use within my company so I can see one
arising in the future.

*Does the API match with current best practices?*

I share Zach Laine's concerns about raw strings (status quo) vs. a more
structured way to pass data to the API. However I'm sympathetic to
arguments that:

   - Redis-like products may extend the set of commands and that's
   important to support
   - Redis itself may extend the set of commands and it'd be a shame to
   force users to wait for a library upgrade (or worse a storm of forks) to
   get support for new features

Additionally I see the request for a more structured way to pass data to
the API as a pure extension: What exists right now is a good lower layer
that something higher level can be built on top of. It's not one or the
other, in a perfect world both would exist.

I think the API should be more generic, for example:

   - Why is std::tuple used rather than supporting anything which supports
   the tuple protocol?
   - std::pmr::string is used internally by aedis::request but the
   documentation suggests that for deserialization purposes only std::string
   is supported (along with only std::vector et cetera)
   - Why the concrete reliance on aedis::resp3::request (rather than any
   type which obeys some protocol)?

I don't think that aedis::basic_connection::reset_stream should exist: It
marries the class too heavily to TCP-like streams and doesn't add anything
beyond being a convenience wrapper for several functions of the underlying
stream. If anything it should be a free function.

Note that the above concern is mirrored somewhat in several parts of the
code where members such as is_open are used.

Several opportunities to add noexcept (conditional or otherwise) should be
taken. I don't see any reason why detail::connection_base::get_executor
shouldn't be noexcept, for example.

I feel as though read and write buffers should be customizable rather than
being married to std::pmr::string (detail/connection_base.hpp). If the
implementation is going to maintain its own buffers I think there should be
a way to inspect them for post mortems (e.g. generating clear and
context-rich error messages).

*Is the documentation helpful and clear?*

Yes, although some things could be made clearer. It was unclear to me at
first glance what it mean to "[r]eset[] the underlying stream"
(documentation for aedis::basic_connection::reset_stream), for example.

*Did you try to use it?*

I did not.

*What is your evaluation of the implementation?*

I'd rather see operator[] (possibly combined with an assert) rather than
at().

I dislike the AEDIS_CHECK_OP# macros (by virtue of the fact that they're
macros and, in my opinion, substantially reduce the readability of the code
at the point where they're used).

In detail/connection_ops.hpp:271 the operation appears to complete
sucessfully because stopping "was requested from the outside." My first
instinct is that in this situation the operation should end with
operation_canceled.

*Please explicitly state that you either accept or reject the inclusion of
this library into Boost*

Accept. I don't think any of the concerns I've raised are breaking and so
they can be addressed later.

Thanks,

--Robert