Turn PERSIST command into variadic.

[huangzhw]

As I know all commands which can be turned into variadic compatibly were done but PERSIST.
In fact variadic PERSIST uses less code. So I think it's reasonable to turn it into variadic.

[oranagra]

AFAIK not all commands that can be turned into variadic were changed.
specifically, considering EXPIRE and TTL were not, so i'm not sure PERSIST should.

For EXPIRE, there's really no problem.

For TTL, we can argue that we can't (easily) since it'll change the response type.
but considering the case of LPOP with COUNT, we can change the response type if the user added arguments.
i.e. respond with simple number when TTL is called with one arg, and with an array if it is called with more or something of that sort. i'm not sure i'm comfortable with this (not explicit enough), but we can figure something out.

however, i'm not sure we wanna go down that road.. specifically about multi-key commands.
IIRC the commands we recently enhanced were about multiple fields in a single key.

@redis/core-team please share your thoughts if you have any..

[huangzhw]

@oranagra What I mean all commands is like DEL/EXISTS/UNLINK. When there is one key, these commands return 0 or 1. When there are multiple keys, these commands return the count of successful operated keys. So the return types are totally Compatible with past.
EXPIRE I'm missed.
TTL we have to return array.

[oranagra]

EXPIRE doesn't need to have the same TTL for all keys, it can use keystep of 2. i.e. EXPIRE key ttl [key ttl ...] like MSET

[huangzhw]

I misunderstand. EXPIRE is OK. I missed it.

[madolson]

The only concern I have is that making this variadic locks down future extensions to the command structure. It is also weird that the inverse, EXPIRE (and the other variants we have now), is not variadic. So I would be more inclined if we were to make all of TTL type commands variadic, which seems complicated based on oran's comment.

Another issue I've recently become aware is that for clients in statically typed languages, it's painful to return different types of responses for the same command, so changing the return type based on the argument is not a great option.

So my net vote is no, let's not do this.

[huangzhw]

I don't like changing the return type based on the argument idea too.
The PERSIST or EXPIRE is just like DEL/EXISTS/UNLINK.

[oranagra]

I think we need to either handle all (EXPIRE, PERSIST, TTL, EXPIREAT, EXPIRETIME), or none.

I'm willing to overlook the concern @madolson shared about blocking future extensions to the command. if needed we'll be forced to add another command.

with regards to clients on typed languages, i suppose they can keep supporting a int ttl(key) for just one key, and add a int[] mttl(keys[]) that calls TTL command with multiple keys, but then when mttl() calls TTL with just one key, we need to return an array.
i.e. in the case of LPOP, without COUNT it returns a string, and with COUNT (even if the count is 1), it returns an array.
with a variadic TTL that's not possible, so a client implementing MTTL by using variadic TTL will have an issue.

so bottom line, i think we should only handle that if we handle all of them, and it seems that it's not that simple to decide on how to do it.
so considering it's not a major issue (user can call these in a pipeline with multi-exec), i think we can waive that.

[huangzhw]

Thanks for reply. At first, I just think make this command variadic is simple and compatible. I didn't think too much deeper. So maybe someday this is really needed we can do it.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}