Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issuing GEORADIUS_RO on a replica fails when no masters are available. #1198

Closed
leif-erikson opened this issue Dec 30, 2019 · 3 comments
Closed
Labels
type: bug A general bug
Milestone

Comments

@leif-erikson
Copy link

Not sure if this is an actual issue or just me being dumb.

I have simple replication group setup with 1 master and 2 replicas (This is with the cluster-enabled config value set to false in the redis instance config). My goal is a have a setup where by default I am able to make READ command to the MASTER, but if it's down, the commands should be sent to the replicas. This becomes a bit tricky for the GEORADIUS command as it's considered a WRITE command by Redis.

Using the StatefulRedisMasterReplicationConnection I am issuing a custom GEORADIUS_RO command with ReadFrom set to MASTER_PREFERRED (there doesn't seem to be any in-built support for GEORADIUS_RO for non-cluster-enabled connection types). This seems to work fine if the master is working, but it fails if the master instance is down, even with the replica being available for the read operation.

I've tested a couple of scenerios whose outputs I've listed below, hopefully they are of some help.

  1. Replication group (1 Master, 2 Replica) -- Master is down. Replica is up.
    Command issued -- GEORADIUS_RO
    Exception raised by lettuce -- io.lettuce.core.RedisException: Currently not connected. Commands are rejected.

    This seems to happen due to the fact that lettuce doesn't have the command GEORADIUS_RO as readonly command in the file ReadOnlyCommands.java and thus it thinks it's a WRITE command and tries to execute it on a MASTER instance (which is not available).

  2. Replication group (1 Master, 2 Replica) -- Master is down. Replica is up.
    Command issued -- GEORADIUS
    Exception raised by lettuce -- io.lettuce.core.RedisCommandExecutionException: READONLY You can't write against a read only replica.

    This seems to fail likely due to the fact the command is sent to the REPLICA instance, but it errors out due to the fact that GEORADIUS is considered a WRITE command as per Redis docs.

I did find a commit which makes GEORADIUS use GEORADIUS_RO internally if available on a cluster, but it's only implemented for the cluster-enabled setups.

For now I've worked around the issue by compiling a local version of lettuce with adding GEORADIUS_RO as a read-only command in the ReadOnlyCommands.java.

Here's a small snippet that can be used to replicate the issue.

RedisClient redisClient = RedisClient.create();

RedisURI host1 = RedisURI.Builder.redis(<MASTER-HOST>, <MASTER-PORT>).withPassword(password).build();
RedisURI host2 = RedisURI.Builder.redis(<REPLICA-1-HOST>, <REPLICA-1-PORT>).withPassword(password).build();
RedisURI host3 = RedisURI.Builder.redis(<REPLICA-2-HOST>, <REPLICA-2-PORT>).withPassword(password).build();

StatefulRedisMasterReplicaConnection<String, String> connection = MasterReplica.connect(redisClient, StringCodec.UTF8, Arrays.asList(host1, host2, host3));

connection.setReadFrom(ReadFrom.MASTER_PREFERRED);

RedisAsyncCommands<String, String> async = redisConnection.async();

GeoWithinListOutput output = new GeoWithinListOutput<>(StringCodec.UTF8, false, false, false);
CommandArgs args = new CommandArgs<>(StringCodec.UTF8).addKey("LOCATION").add(longitude).add(latitude).add(radius).add(GeoArgs.Unit.m.toString());
GeoArgs geoArgs = (new GeoArgs()).asc().withCount(250);
geoArgs.build(args);

RedisFuture<List<GeoWithin<String>>> locations = async.dispatch(CommandType.GEORADIUS_RO, output, args);

Regards,
Leif

@leif-erikson leif-erikson added the type: bug A general bug label Dec 30, 2019
@mp911de
Copy link
Collaborator

mp911de commented Dec 30, 2019

We added GEORADIUS_RO/GEORADIUSBYMEMBER_RO support to Redis Cluster initially as cluster connection initialization on Redis cluster is somewhat expensive and querying the Redis command set does not impact so much the connection creation, compared to standalone Redis. We should add both commands to the Master/Replica API ReadOnly command definition.

The ReadOnly variant is only required for Read-only nodes so I'm not sure whether it makes sense to add that kind of overhead to standalone Redis operations. Master/Replica connections use the same type of connections as standalone Redis as Master/Replica affects only the routing without further cluster-awareness.

@mp911de mp911de added this to the 5.2.2 milestone Jan 4, 2020
mp911de added a commit that referenced this issue Jan 4, 2020
Both commands can now be routed to read-only connections when using Redis Cluster or Master/Replica.
mp911de added a commit that referenced this issue Jan 4, 2020
Both commands can now be routed to read-only connections when using Redis Cluster or Master/Replica.
@mp911de
Copy link
Collaborator

mp911de commented Jan 4, 2020

GEORADIUS_RO and GEORADIUSBYMEMBER_RO are now enabled for replica routing. Likely, we will introduce in a future revision the support of CommandSet to determine command properties from Redis' COMMAND metadata.

@mp911de mp911de closed this as completed Jan 4, 2020
@leif-erikson
Copy link
Author

@mp911de Thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants