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

Geosearch and FCALL_RO commands go to the master node #2568

Closed
kajte opened this issue Dec 12, 2023 · 2 comments
Closed

Geosearch and FCALL_RO commands go to the master node #2568

kajte opened this issue Dec 12, 2023 · 2 comments
Labels
status: good-first-issue An issue that can only be worked on by brand new contributors type: bug A general bug
Milestone

Comments

@kajte
Copy link
Contributor

kajte commented Dec 12, 2023

Bug Report

Current Behavior

geosearch commands go to master node even if read intention ReadFrom.REPLICA is specified.

Input Code

  1. Start simple master/replica redis setup with docker-compose
  2. Connect to both redis nodes and run MONITOR command to see commands coming to nodes
  3. Run java code
  4. Observe that geosearch command goes to primary node and get replica node
# docker-compose.yaml
version: "3.9"
services:
  redis-master:
    image: "redis"
    ports:
      - "6379:6379"
    expose:
      - "6379"
  redis-replica:
    image: "redis"
    ports:
      - '6380:6379'
    command: redis-server --slaveof redis-master 6379
// Main.java
package org.example;

import io.lettuce.core.*;
import io.lettuce.core.codec.StringCodec;
import io.lettuce.core.masterreplica.MasterReplica;
import io.lettuce.core.masterreplica.StatefulRedisMasterReplicaConnection;

import java.util.List;

public class Main {
    public static void main(String[] args) {
        RedisURI masterUri = RedisURI.create("redis://localhost:6379");
        RedisURI replicaUri = RedisURI.create("redis://localhost:6380");

        RedisClient client = RedisClient.create();
        List<RedisURI> nodes = List.of(masterUri, replicaUri);
        StatefulRedisMasterReplicaConnection<String, String> connection = MasterReplica.connect(client, StringCodec.UTF8, nodes);

        connection.setReadFrom(ReadFrom.REPLICA);
        connection.sync().get("key");
        connection.sync().geosearch("another-key", GeoSearch.fromCoordinates(0.0, 0.0), GeoSearch.byRadius(1.0, GeoArgs.Unit.km));
    }
}

Expected behavior/code

geosearch commands should go to replica node when read intention ReadFrom.REPLICA is specified as the command (https://redis.io/commands/geosearch/) is only reading.

Environment

  • Lettuce version: Tested with 6.3.0.RELEASE but I believe other versions are affected too
  • Redis version: 7.2.3

Possible Solution

Add geosearch to the list of read_only commands similar to #2198 did for smismember. I can open a PR to do that.

kajte added a commit to kajte/lettuce-core that referenced this issue Dec 12, 2023
Set GEOSEARCH as read-only command to allow running the command on replicas.
@mp911de mp911de added type: bug A general bug status: good-first-issue An issue that can only be worked on by brand new contributors labels Dec 13, 2023
@thx01138
Copy link

The same goes for FCALL_RO. My use case requires these to be spread among ReadFrom.ALL but currently they only go to masters.

@mp911de
Copy link
Collaborator

mp911de commented Dec 13, 2023

As workaround, you can configure ClientOptions.readOnlyCommands by providing a ReadOnlyPredicate including the affected commands.

@mp911de mp911de changed the title Geosearch commands go to the master node Geosearch and FCALL_RO commands go to the master node Jan 3, 2024
@mp911de mp911de added this to the 6.3.1.RELEASE milestone Jan 3, 2024
mp911de pushed a commit that referenced this issue Jan 3, 2024
Set GEOSEARCH as read-only command to allow running the command on replicas.

Original pull request: #2569
mp911de added a commit that referenced this issue Jan 3, 2024
mp911de pushed a commit that referenced this issue Jan 3, 2024
Set GEOSEARCH as read-only command to allow running the command on replicas.

Original pull request: #2569
mp911de added a commit that referenced this issue Jan 3, 2024
@mp911de mp911de linked a pull request Jan 3, 2024 that will close this issue
@mp911de mp911de closed this as completed Jan 3, 2024
ggivo added a commit to ggivo/lettuce that referenced this issue Oct 30, 2024
…lica" on READS... only if master & replica configured redis#1813

Divert pure read intentions of georadius and georadiusbymember commands (variants that do not use STORE/STOREDIST) to GEORADIUS_RO/GEORADIUSBYMEMBER_RO
This will unify the behaviour between Cluster and Redis Standalone/Replica arrangements

Relates to  issues redis#1481 redis#2568 redis#2871

Closes redis#1813
tishun pushed a commit that referenced this issue Nov 5, 2024
…lica " on READS... (#3032)

* OpsForGeo producing "READONLY You can't write against a read only replica" on READS... only if master & replica configured #1813

Divert pure read intentions of georadius and georadiusbymember commands (variants that do not use STORE/STOREDIST) to GEORADIUS_RO/GEORADIUSBYMEMBER_RO
This will unify the behaviour between Cluster and Redis Standalone/Replica arrangements

Relates to  issues #1481 #2568 #2871

Closes #1813

* Fix tests

* Remove unused methods

* Fix tests and add tests  withArgs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: good-first-issue An issue that can only be worked on by brand new contributors type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants