Improve the `MGet` helper for the cluster client by using `MGET` commands

[fabiomargarido]

Hey there.

I'm a bit confused by the wording in the comment and README regarding the MGet helper functions:

// MGet is a helper that consults the redis directly with multiple keys by grouping keys within the same slot into MGET or multiple GETs
func MGet(client Client, ctx context.Context, keys []string) (ret map[string]RedisMessage, err error) {

Specifically, the part where it says keys are grouped taking the slot into consideration, since looking at the code it seems it simply checks if the client is not a cluster client, in which case an MGET is sent, and falls back to a pipelined GET for each separate key when it is in fact a cluster.

I was wondering if it would be possible to pre-calculate the target slot for each key and send a separate MGET command to each node responsible the a particular set of slots. If this fails for some reason (e.g. cluster topology changed and refresh was not yet executed), then fallback to the current implementation.

Thanks.

[rueian]

Hi @fabiomargarido,

You are right. Currently, the helper will simplify pipeline GETs to the cluster client. I think it is a good idea to group them into multiple MGETs.

Would you like to give it a try? intl.Slot() can do the pre calculation.

[fabiomargarido]

Thanks for the quick reply. I'm actually a bit more puzzled now, since I found this commit from almost 2 years ago:

086a244

It seems this is how it worked before, but was changed for performance reasons. Is there anything you think could be done differently now to improve it?

[rueian]

Hi @fabiomargarido,

Thanks for bringing up the old commit. Previous to the commit, we did parallelMGet which sent MGET for each slot parallelly using multiple DoMulti, but that was too fine-grained. Therefore, after the commit, we simplify put all GETs to one DoMulti and let it do pipelining by connections.

I wonder if we can put MGETs to one DoMulti? For example, if the input of the MGet helper is []string{"{1}a", "{2}a", "{1}b", "{2}b"}, then we put MGET {1}a {1}b and MGET {2}a {2}b to one DoMulti.

[fabiomargarido]

That could make sense... Furthermore, instead of sending one MGET per slot, wouldn't it make sense to check the slot against the range for each node and send one per node?

[rueian]

I want to say it makes sense too but I think multiple slots in one MGET is forbidden by redis server.

[fabiomargarido]

Ah, you're right 😕

[fabiomargarido]

@rueian I'm not quite sure when I'd be able to try and tackle this, so if you have some bandwidth and can give it a shot, it'd be greatly appreciated. Otherwise I'll try at some point.