Re: aedis review starts today

From: Peter Dimov (pdimov_at_[hidden])
Date: 2023-01-18 18:48:15

Zach Laine wrote:
> >  - 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.
> };

Defining a few hundred enumerators doesn't strike me as particularly fruitful
exercise, even if they were static and guaranteed to cover every command
(they are neither.)

Plus, it doesn't even gain that much. You catch spelling errors in the command
names, but nothing else.

...

> void user_code_1()
> {
>     resp3::request req;
>     req.push(HELLO(3));
>     req.push(HGETALL("hset-key"));
>     req.push(QUIT);
> }

That's better, but there's no need to make HELLO et al variables, you could
just have

struct redis::cmd_hello
{
    int protover;
};

and then

    req.push( redis::cmd_hello{ 3 } );

but even then, HELLO is actually

    HELLO [protover [AUTH username password] [SETNAME clientname]]

so the struct can't be as simple, and the majority of the Redis commands
don't map well to C++.

Plus, note that the above says resp3::request, that is, this is a generic RESP3
request; disallowing the low-level stringly typed interface will make this not
work for other RESP3 servers, and there's _probably_ room in Boost for a
generic RESP3 client.

A specialized Redis client based on type-safe C++ structs for the commands
will no doubt be useful, but I'm not sure maintaining one by hand is the
right way to go. Taking

https://github.com/redis/redis/tree/4ffcec29af9dc1246540d94834540528576c68a4/src/commands

and automatically generating the C++ glue from that may be more
manageable.