search

vert-x3 / vertx-redis-client

Redis client for Vert.x
http://vertx.io
Apache License 2.0
128 stars 116 forks source link

Command.XREADGROUP error MOVED #440

Open fmeneghetti opened 5 months ago

fmeneghetti commented 5 months ago

I am experiencing some issues in Redis Cluster mode, particularly with the MOVED error. It is still unclear to me how to transparently handle this when the cluster is unbalanced.

I think there is a problem with the "xreadgroup" command, though, because unlike the "xread" command, which is a read command, "xreadgroup" is a write command.

In RedisReplicas.SHARE or RedisReplicas.ALWAYS mode I run into the MOVED error.

I suppose it is to treat the command as "master only":

addMasterOnlyCommand(XREADGROUP);

vertx-redis-client -> 4.5.6

Ladicek commented 5 months ago

All commands that are not read-only are routed to master nodes already, so that is not an issue.

I noticed a while ago that we transparently handle ASKED redirections, but propagate MOVED redirections as errors. I don't know why we do that, the Redis cluster design assumes that clients handle MOVED redirections as well. That is an issue we have, I believe.

fmeneghetti commented 5 months ago

I think there is a problem with the "xreadGroup" command because it is recognized as "readOnly" and therefore is also sent to the replication nodes.

fmeneghetti commented 5 months ago

Command XREADGROUP = new CommandImpl("xreadgroup", -7, false, false, false, new KeyLocator(true, new BeginSearchKeyword("STREAMS", 4), new FindKeysRange(-1, 1, 2)));

The first param is Boolean readOnly

Ladicek commented 5 months ago

You might want to debug CommandImpl.isReadOnly(), which determines if the command is actually treated as read-only, based on argument values first.

fmeneghetti commented 5 months ago

I did it:

15:58:46.761 [vert.x-eventloop-thread-2] INFO i.m.s.v.tst.pri.TstDeployerVerticle - XREADGROUP GROUP mgg.signal.workers mgg.signal.worker:1 COUNT 1 BLOCK 1000 STREAMS stream:message > 15:58:46.761 [vert.x-eventloop-thread-2] INFO i.m.s.v.tst.pri.TstDeployerVerticle - command=xreadgroup 15:58:46.761 [vert.x-eventloop-thread-2] INFO i.v.r.c.impl.RedisClusterConnection - [redis]keySlot=1163 15:58:46.761 [vert.x-eventloop-thread-2] INFO i.v.r.c.impl.RedisClusterConnection - [redis]readOnly=true 15:58:46.761 [vert.x-eventloop-thread-2] INFO i.v.r.c.impl.RedisClusterConnection - [redis]endpoints=[redis://172.26.0.101:7001, redis://172.26.0.105:7005] 15:58:46.761 [vert.x-eventloop-thread-2] INFO i.v.r.c.impl.RedisClusterConnection - [redis]endpoint=redis://172.26.0.105:7005 15:58:46.761 [vert.x-eventloop-thread-2] ERROR i.v.r.c.impl.RedisClusterConnection - [redis]MOVED 1163 172.26.0.101:7001

fmeneghetti commented 5 months ago

After changing the "ro" parameter of the KeyLocator class, I saw that things are working fine:

Command XREADGROUP = new CommandImpl("xreadgroup", -7, false, false, false, new KeyLocator(false, new BeginSearchKeyword("STREAMS", 4), new FindKeysRange(-1, 1, 2)));

16:04:49.681 [vert.x-eventloop-thread-2] INFO i.m.s.v.tst.pri.TstDeployerVerticle - XREADGROUP GROUP mgg.signal.workers mgg.signal.worker:1 COUNT 1 BLOCK 1000 STREAMS stream:message > 16:04:49.681 [vert.x-eventloop-thread-2] INFO i.m.s.v.tst.pri.TstDeployerVerticle - command=xreadgroup 16:04:49.681 [vert.x-eventloop-thread-2] INFO i.v.r.c.impl.RedisClusterConnection - [redis]keySlot=1163 16:04:49.681 [vert.x-eventloop-thread-2] INFO i.v.r.c.impl.RedisClusterConnection - [redis]readOnly=false 16:04:49.681 [vert.x-eventloop-thread-2] INFO i.v.r.c.impl.RedisClusterConnection - [redis]endpoints=[redis://172.26.0.101:7001, redis://172.26.0.105:7005] 16:04:49.681 [vert.x-eventloop-thread-2] INFO i.v.r.c.impl.RedisClusterConnection - [redis]endpoint=redis://172.26.0.101:7001

now in the log is printed correctly: "readOnly=false"

Ladicek commented 5 months ago

Funny. The command descriptors are automatically generated from Redis COMMAND output (https://redis.io/commands/command/), so should be correct. Maybe we have a bug in the isReadOnly logic, I'd have to take a proper look, I didn't have to dig into that deeply yet.

fmeneghetti commented 3 months ago

Hi @Ladicek, do you have news? Thanks.

Ladicek commented 3 months ago

I don't have any news, no.