gosquared / redis-clustr

Redis Cluster client for Node.js
MIT License
107 stars 25 forks source link

TIME doesn't work because it has no key #25

Closed lukemcgregor closed 6 years ago

lukemcgregor commented 6 years ago

TIME doesn't have a key, and can run identically on any node.

The code uses the key to select the correct shard: https://github.com/gosquared/redis-clustr/blob/master/src/RedisClustr.js#L362

But errors out with no key for command: time.

I think instead of erroring here it should just pick a shard at random and execute there. Im not sure if there are other scenarios where commands might be accidentally missing keys and require this error. If so then add a keyless config setting and pick at random (or shard 0) then.

jbt commented 6 years ago

Hi @lukemcgregor, that's a good shout. I think originally we didn't have support for keyless commands because most of them (e.g. anything CONFIG or CLUSTER-related) don't make sense to run against a cluster (you'd most likely want to be issueing them against specific individual servers)

TIME, though, could make sense, at least to make it so it doesn't error. Although it should probably come with a word of caution that the nodes in your cluster won't necessarily report exactly the same time, so you probably shouldn't rely on it for absolute accuracy. I'll make a PR and this can go into a new release

(out of curiosity, do you have a particular use-case for the TIME command? e.g. is it something that might break if we select a shard at random vs always addressing shard 0?)

lukemcgregor commented 6 years ago

@jbt Yeah I'm not to sure about all the use cases for TIME, my particular one was that I had it inside a health check to check the downstream redis system was running. When I switched over to a clustered version of redis I got this error.

Picking the right instance will make this a bit weird. You're totally right that each will return subtly different time and I can imagine this being useful for some applications. Maybe instead the command should return an array of times from every instance or perhaps take an optional argument to select a specific node? Might be worth checking out other language libraries and what they do here as they might have a good answer?

jbt commented 6 years ago

By the look of things we already thought about handling keyless commands by selecting a random client, but the error is getting thrown before ever reaching there.

My guess is that something changed and inadvertently made that code redundant, but I'm not sure. I'll try and figure out what the intention was with that change, because it should hopefully make handling TIME a fairly minor change

jbt commented 6 years ago

So as it turns out, that line of code has flagged up another rather fun bug: #26. So thanks for prompting me to dig into this, I'll try and fix the two together

jbt commented 6 years ago

Just published v1.6.0 with the changes for #26 and this. Things like TIME seem to behave now as far as I can tell, routing to a random node as always intended.