This review was sent directly to me.
---------- Forwarded message ---------
From: Robert A.H. Leahy
Date: Tue, Jan 24, 2023 at 1:04 PM
Subject: Aedis Review
To: klemensdavidmorgenstern@gmail.com
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
