simplified command execution - the Connection is an implementation de… by arnead · Pull Request #2 · sewenew/redis-plus-plus (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

…tail

Usage example:
Redis redis("tcp://127.0.0.1:6379");
char const* clientName = "Hugo";
redis.command("CLIENT SETNAME %s",clientName);

…tail

Usage example: Redis redis("tcp://127.0.0.1:6379"); char const* clientName = "Hugo"; redis.command("CLIENT SETNAME %s",clientName);

Usage example:
Redis redis("tcp://127.0.0.1:6379");
char const* clientName = "Hugo";
redis.command("CLIENT SETNAME %s",clientName);

arnead deleted the cpp14-simple-string-command branch

December 13, 2018 08:10

arnead restored the cpp14-simple-string-command branch

December 13, 2018 08:10

Maybe it would be a good idea to provide a cmake switch CPP_11 and enable the simplified version only, if that switch is not set. (Of course, additionally the cxx flag should be controlled by that switch).

Hi @arnead First of all, thanks a lot for your pull request!

Yes, you're right. Connection is implementation detail, and it should NOT be exposed to end users. I design this complicate interface for the following reason:

User defined Cmd function, e.g. void set_udf(const StringView &key, const StringView &val), can be reused for both Redis::command and RedisCluster::command. So that we can call redis.command(set_udf, "key", "val") and redis_cluster.command(set_udf, "key", "val") without a duplicate "set %s %s".

I have to say this interface is NOT user-friendly, and I was thinking about having a simplified version.

Your proposal, i.e. redis.command("set %s %s", "key", "val"), is good for Redis. However, it CANNOT work with RedisCluster very well. For RedisCluster, we need to automatically get the key of the command, so that we can send it to the right node. With your proposal, we CANNOT easily achieve the goal.

A better interface might be something like: redis.command("set", "key", "val") and redis_cluster.command("set", "key", "val"). For RedisCluster, we can easily get the key of the command, i.e. the second parameter of RedisCluster::command (by now, RedisCluster doesn't support command without a key as parameter).

I'll try to implement it this week. If you have any problem or other proposal, please feel free to let me know. Thanks again for your work!

Hi sewenew, the example you mention does not need to be invoked as „command“ by the end user – for set the user can call the function bool set(const StringView &key, const StringView &val, const std::chrono::milliseconds &ttl = std::chrono::milliseconds(0), UpdateType type = UpdateType::ALWAYS) which exists for both RedisCluster and Redis. Additionally, my proposal simply adds an overload of the command function to Redis. This could be done for RedisCluster as well, provided that there is a non-empty set of commands which can be executed as formatted string. And I don’t understand, why it is easier to get the „set“ semantic of a command if it is wrapped in a function. redis.command(function,StringView,StringView) can be any redis command that accepts two strings, for instance CONFIG SET parameter value. So, whenever a command needs to be handled differently by a redis cluster client, you can solve that by adding an explicit function interface to both Redis and RedisCluster (which you already have for set). Kind Regards, Arne Adams Von: sewenew notifications@github.com Gesendet: Donnerstag, 13. Dezember 2018 17:32 An: sewenew/redis-plus-plus redis-plus-plus@noreply.github.com Cc: arnead arne.adams@t-online.de; Mention mention@noreply.github.com Betreff: Re: [sewenew/redis-plus-plus] simplified command execution - the Connection is an implementation de… (#2) Hi @arnead <https://github.com/arnead> First of all, thanks a lot for your pull request! Yes, you're right. Connection is implementation detail, and it should NOT be exposed to end users. I design this complicate interface for the following reason: User defined Cmd function, e.g. void set_udf(const StringView &key, const StringView &val), can be reused for both Redis::command and RedisCluster::command. So that we can call redis.command(set_udf, "key", "val") and redis_cluster.command(set_udf, "key", "val"). I have to say this interface is NOT user-friendly, and I was thinking about having a simplified version. Your proposal, i.e. redis.command("set %s %s", "key", "val"), is good for Redis. However, it CANNOT work with RedisCluster very well. For RedisCluster, we need to automatically get the key of the command, so that we can send it to the right node. With your proposal, we CANNOT easily achieve the goal. A better interface might be something like: redis.command("set", "key", "val") and redis_cluster.command("set", "key", "val"). For RedisCluster, we can easily get the key of the command, i.e. the second parameter of RedisCluster::command (by now, RedisCluster doesn't support command without a key as parameter). I'll try to implement it this week. If you have any problem or other proposal, please feel free to let me know. Thanks again for your work! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AkGuP6AovRIA1ZKIbsW22t5BCyEU7RtTks5u4oDkgaJpZM4ZRHqu> . <https://github.com/notifications/beacon/AkGuP786MMV0vJPR0oq1-osuwDX43p7Cks5u4oDkgaJpZM4ZRHqu.gif>

Hi Arne,

I'm not a native speaker, and my previous comment might be confusing for you. Sorry for that, and I'll try to explain in detail.

the example you mention does not need to be invoked as „command“ by the end user

Yes, you don't need to invoke Redis::command to send a set command. I only use the set command as an example.

my proposal simply adds an overload of the command function to Redis. This could be done for RedisCluster as well

When we send command to Redis Cluster, we need to do the following work:

get the key of the command
calculate the slot number of the key, i.e. slot = CRC16(key) % 16384.
look up the slot-node mapping to find out where is the key located.
send the command to the right node.
If the command doesn't have a key, e.g. CLIENT SETNAME name. End user needs to call Redis RedisCluster::redis(const StringView &hash_tag) to get a connection to the node of the giving hash_tag. Then send the command with the returned Redis instance. I'm still testing this interface, and hasn't committed.

If we overload RedisCluster::command with a format string and a list of arguments, i.e. ReplyUPtr command(const StringView & fmt, Args &&...args). This overload method cannot figure out to which node we should send the command, since we have no idea how the end user will construct his format string, and it's hard to parse the key from the argument list. End user might invoke it as: cluster.command("set %s %s", "key", "val") or cluster.command("%s %s %s", "set", "key", "val"). The key of the command might be the second parameter or the third parameter.

And I don’t understand, why it is easier to get the „set“ semantic of a command if it is wrapped in a function. redis.command(function,StringView,StringView) can be any redis command that accepts two strings, for instance CONFIG SET parameter value.

In my previous comment, I gave an example of a simpler interface, and didn't provide the prototype, and it might make you confused. The prototype should be something like: ReplyUPtr command(const StringView &command, Args &&...args). The first argument is the command string. If the command has a key, e.g. the SET command: cluster.command("SET", "key", "val"), the key must be the first argument of the args list, and we can easily get the key when we implement RedisCluster::command. If the command doesn't have a key, e.g. the CLIENT SETNAME command, end user and call RedisCluster::redis(hash_tag) to get an Redis instance connecting to the right node, and send the command with that instance: cluster.redis(hash_tag).command("CLIENT SETNAME", "name").

That's all. I hope this comment explains my thoughts, and won't confuse you any more :)

Regards

Hi Arne,

I add an easy-to-use command interface for Redis, RedisCluster, Pipeline and Transaction. You can easily pass parameters of string type or arithmetic type to the command interface. For example:

redis.command("set", "key", 123);    // same as redis.set("key", "123");
auto r = redis.command("incr", "key")
cout << reply::parse<long long>(*r) << endl;

redis.command("client", "setname", "name");
r = redis.command("client", "getname");
auto name = reply::parse<OptionalString>(*r);
if (val) cout << *name << endl;

r = redis.command("mget", "k1", "k2", "k3");
vector<OptionalString> result;
reply::to_array(*r, back_inserter(result));

redis_cluster.command("set", 123, "value");   // same as redis_cluster.set("123", "value")
redis_cluster.command("mget", "{key}123", "{key}456", "{key}789");

redis.pipeline().command("set", 1, 1).command("set", 2, 2).exec();  // same as redis.pipeline().set("1", "1").set("2", "2").exec()

Please check the latest code for detail, and hope this interface can be helpful :)

Regards

For most of time, it's redundant to call reply::parse manually. I might modify the interface to let the command interface return the result type directly. For example:

auto val = redis.command("get", "key");
if (val) cout << *val << endl;

vector<OptionalString> res;
redis.command("mget", "k1", "k2", "k3", back_inserter(res));

Sorry for the inconvenience.

Hi Sewenew,
I checked out the master branch at "add an easy-to-use command interface.".
First off, on ubuntu with GCC 5.4 I do get some warnings:
reply.cpp:34:30: error: narrowing conversion, reply.cpp:84:19: error: comparison between signed and unsigned integer expressions
Those warnings result in errors, due to the -Werror switch.
I'm not completely happy with the current string command support.
Instead of
client->command("CLIENT", "SETNAME", clientName.c_str())
I'd rather want to write:
client->command("CLIENT SETNAME", clientName.c_str())
because
"CLIENT SETNAME" connection-name
is the syntax documented in https://redis.io/commands/client-setname

I think your idea to return the result directly (as an Optional where applicable) is great.

Hi Arne,

I finally figured out why you got those error messages: you installed some old version of hiredis :). Old version hiredis defines redisReply::len as int. So when redis-plus-plus tries to construct a string with {reply.str, reply.len}, or compare reply.len with string::size, the compiler complains. The latest version of hiredis has changed the type of redisReply::len to std::size_t. So if you compile the code with the latest hiredis, you won't get any error message.

I've fixed the problem to make the code compatible with old version hiredis, and now the minimum version requirement for hiredis is v0.12.1. Please try the latest code. Thanks a lot for finding the bug!

I'd rather want to write: client->command("CLIENT SETNAME", clientName.c_str())

First of all, you don't need to call c_str, you can just pass a string to the command method:

string clientName("name");
client->command("client", "setname", clientName);

With CLIENT SETNAME connection-name, the command is, in fact, CLIENT, NOT CLIENT SETNAME. SETNAME is just one of the two arguments of this command, or you can take SETNAME as a subcommand. So I prefer to pass CLIENT and SETNAME separately.

Also, if we support command("CLIENT SETNAME", clientName), end user might think they can also call it like this: command("CLIENT SETNAME client-name"). But that's not the case.

Regards

Hi Arne,

I've implemented the generic command interface, and you can use it like this:

redis.command<void>("client", "setname", "name");
auto val = redis.command<OptionalString>("client", "getname");
auto num = redis.command<long long>("incrby", "key", 1);
std::vector<OptionalString> result;
redis.command("mget", "k1", "k2", "k3", std::back_inserter(result));

Hope you like it.

Regards

simplified command execution - the Connection is an implementation de… by arnead · Pull Request #2 · sewenew/redis-plus-plus (original) (raw)

Conversation

2 participants