|
|
Boost : |
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2023-01-17 19:39:12
> On Sun, Jan 15, 2023 at 9:17 AM Klemens Morgenstern via Boost <boost_at_[hidden]> wrote:
>
[snip]
> 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?
I think so, yes.
> - Do you have an application for this library?
I do not.
> - Does the API match with current best practices?
No. In particular, the API requires the user to write lots of Redis
strings by hand. A typo will not be detected until runtime. The set
of Redis keywords is fixed; there's no reason not to make an
enumeration of them all, like this:
enum class cmd {
ACL_CAT, // List the ACL categories or the commands inside a category
ACL_DELUSER, // Remove the specified ACL users and the associated rules
ACL_DRYRUN, // Returns whether the user can execute the given command
// without executing the command.
ACL GENPASS, // Generate a pseudorandom secure password to use for ACL users
ACL_GETUSER, // Get the rules for a specific ACL user
ACL_LIST, // List the current ACL rules in ACL config file format
ACL_LOAD, // Reload the ACLs from the configured ACL file
ACL_LOG, // List latest events denied because of ACLs in place
// ... etc.
};
I actually did all of them to see how much effort it would take. It
took me about 20 or 30 minutes, working from the list on the Redis
website (I just copy/pasted the commands listed at
https://redis.io/commands/).
Further, when enqueuing commands, there's no checking that a command
and its arguments make sense together. For instance, here is some
example code from the docs:
resp3::request req;
req.push("HELLO", 3);
req.push("HGETALL", "hset-key");
req.push("QUIT");
If I change that last line to:
req.push("QUITE");
or to:
req.push("QUIT", "oops");
or even to:
auto it = 42, getting = 13;
req.push("QUIT", this, is, getting, "ridiculous");
... the code is well formed, but nonsensical. I'd like the lib to
know that "QUITE" isn't a thing, and that "QUIT" takes no arguments,
and fail to compile if I mess any of this up via typo or copy/paste.
One possible fix would be to change resp3::request::push*() to take a
non-type template parameter that can be used to check the args:
template<cmd command, class... Ts>
void push(Ts const &... args)
{
static_assert(args_are_compatible_v<command, std::remove_cvref_t<Ts>...>);
// Do the real work here...
}
template<cmd command, class Key, class ForwardIterator>
void push_range(
Key const & key,
ForwardIterator begin,
ForwardIterator end,
typename std::iterator_traits<ForwardIterator>::value_type * = nullptr)
{
static_assert(key_and_range_are_compatible_v<
command,
std::remove_cvref_t<Key>,
ForwardIterator>);
// Do the real work here...
}
A more thorough-going change that I would like even more would be this:
template<cmd c, typename... Params>
struct command_t
{
static constexpr cmd command = c;
command_t(Params &&... params) params_{(Params &&) params} {}
std::tuple<value_or_reference_wrapper_t<Params>...> params_;
};
std::string_view string_name(cmd c) { return /* mapping from cmd to
strings must exist somewhere, obvs. */; }
// value_or_reference_wrapper_t should produce a reference_wrapper<T> when
// std::is_reference_v<T> is true, and T otherwise. IOW, move rvalue
// references to small things like ints and std::string_views into the
// command_t, and refer to lvalue references like std::vector<int> const &
// (examples below).
inline constexpr struct HELLO_t
{
using return_type = /* whatever HELLO returns from Redis, as a C++ type */;
// HELLO might need multiple overloads, and/or to derive from command_t<...>
// (see below); the https://redis.io/commands description says it
takes optional parameters.
auto operator()(int param)
{
return command_t<cmd::HELLO, int>(std::move(param));
}
} HELLO;
inline constexpr struct HGETALL_t
{
using return_type = /* whatever HGETALL returns from Redis, as a
C++ type */;
auto operator()(std::string_view param)
{
return command_t<cmd::HGETALL, int>(std::move(param));
}
} HGETALL;
// "nullary" commands should inherit from command_t<cmd::NAME> to avoid
// superfluous parens at the call site.
inline constexpr struct QUIT_t : command_t<cmd::QUIT>
{
using return_type = /* whatever QUIT returns from Redis, as a C++ type */;
} QUIT;
// etc. All the commands need this treatment. If you want to keep the result
// type flexible, you can make return_type a kind-enumerator instead. For
// instance, instead of std::vector<int> it might be
// return_kind::number_array, where return_kind is some enumeration. The
// kind-enumumerator would need to be static constexpr. Though this looks
// like a lot of work, you can pretty easily make macros that do all this.
template<cmd c, typename... Params>
void resp3::request::push(command_t<c, Params...> command)
{
// Process the command as the code previously did, except now the params
// *must* match the command, because of how the command_t is constructed
// at the call site.
}
void user_code_1()
{
resp3::request req;
req.push(HELLO(3));
req.push(HGETALL("hset-key"));
req.push(QUIT);
}
// Even better, we can write this directly:
void user_code_v2()
{
// No need to allocate a request object for smallish requests; you only
// need to allocate the underlying buffer that the bytes are serialized
// into. If you insist, you can always overload with an initial allocator
// param.
co_await conn->async_exec_request(HELLO(3), HGETALL("hset-key"), QUIT);
}
void user_code_2()
{
using exec_resp_type = std::tuple<
std::optional<std::string>, // get
std::optional<std::vector<std::string>>, // lrange
std::optional<std::map<std::string, std::string>> // hgetall
>;
std::tuple<
aedis::ignore, // multi
aedis::ignore, // get
aedis::ignore, // lrange
aedis::ignore, // hgetall
exec_resp_type, // exec
>
resp;
// Since each command is a single object, we can also do this:
co_await conn->async_exec_transaction(
MULTI, GET("key1"), LRANGE("key2", 0, -1), HGETALL("key3"), EXEC, resp);
// resp might need to be the first param, but you get the idea.
// As before, we don't actually need to allocate a request object, just
// the buffer of bytes that it would normally fill.
// We can also use the statically-available return type (or return kind
// enumerator) of each arg, and knowledge of the fact that this is
a transaction, to
// static_assert that the type of resp is compatible.
}
// Even better, we can use the fact that resp is the only meaningful value to
// allow the user to write this instead:
void user_code_2_v2()
{
using exec_resp_type = std::tuple<
std::optional<std::string>, // get
std::optional<std::vector<std::string>>, // lrange
std::optional<std::map<std::string, std::string>> // hgetall
>;
exec_resp_type resp;
co_await conn->async_exec_transaction(
MULTI, GET("key1"), LRANGE("key2", 0, -1), HGETALL("key3"), EXEC, resp);
}
These are the kinds of easy-to-use-correctly and
hard-to-use-incorrectly interfaces that I would expect of such a
library. I think the marshalling/unmarshalling and handling of ASIO
parts of the existing design make my life easier, but with raw string
literals in the interface, I consider the library interface work only
half-done.
> - Is the documentation helpful and clear?
It is geared more towards users who already know all about Redis, and
it is more reference-oriented. There is a real lack of the
tutorial-style docs that Boost is known for.
Boost reaches a really wide audience. There are a lot of people who
might not be very DB-savvy, who might want to use Aedis, since "Hey, I
already have Boost, why not use this DB library?" There are no docs
for such users.
For Redis aficionados, there should also be a step-by-step guide to
how to do real work with the lib. The examples seem pretty good. I
think one or more of them need to be integrated into the docs as a
tutorial. There are lots of examples of other Boost libs that do
this.
The reference documentation is often too spare. For instance:
I must convert data to a string. Are there any requirements on the
string? Can it have internal nulls, characters requiring escaping,
etc.? I don't necessarily think that the docs need to talk about this
in the reference section, but since this is not covered in the
also-spare tutorial section, how am I supposed to know what the string
requirements are? The Serialization section in
https://mzimbres.github.io/aedis/index.html is similarly problematic.
This is the example:
// User defined type.
struct mystruct {...};
// Serialize it in to_bulk.
void to_bulk(std::pmr::string& to, mystruct const& obj)
{
std::string dummy = "Dummy serializaiton string.";
aedis::resp3::to_bulk(to, dummy);
}
I was baffled by this at first reading. Why is obj there? It is
unused. If "Dummy serializaiton string." were replaced with /*
stringify obj here */, the example makes sense. I did not understand
that I was supposed to do that until I read the reference mentioned
earlier.
There are a lot of things like this in the docs -- more than I have
time to list. I don't think the docs are up to the Boost standard.
> - Did you try to use it? What problems or surprises did you encounter?
I did not.
> - What is your evaluation of the implementation?
I did not look at much of the implementation. What little I did see
seemed reasonable.
> Additionally, stating your knowledge of redis and asio would be helpful for
> me.
I've used ASIO a lot, and never used Redis.
I vote to REJECT.
If the library could be refactored so that the user-facing interface
was more friendly, I would change my vote.
Zach
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk