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

georaduis command execute error when use ReadFrom.REPLICA_PREFERRED #2832

Closed
kennethfan opened this issue Apr 18, 2024 · 12 comments
Closed

georaduis command execute error when use ReadFrom.REPLICA_PREFERRED #2832

kennethfan opened this issue Apr 18, 2024 · 12 comments
Labels
status: good-first-issue An issue that can only be worked on by brand new contributors
Milestone

Comments

@kennethfan
Copy link

Bug Report

Current Behavior

Stack trace
org.springframework.data.redis.RedisSystemException: Error in execution; nested exception is io.lettuce.core.RedisReadOnlyException: READONL
Y You can't write against a read only replica.
        at org.springframework.data.redis.connection.lettuce.LettuceExceptionConverter.convert(LettuceExceptionConverter.java:54)
        at org.springframework.data.redis.connection.lettuce.LettuceExceptionConverter.convert(LettuceExceptionConverter.java:52)
        at org.springframework.data.redis.connection.lettuce.LettuceExceptionConverter.convert(LettuceExceptionConverter.java:41)
        at org.springframework.data.redis.PassThroughExceptionTranslationStrategy.translate(PassThroughExceptionTranslationStrategy.java:44)
        at org.springframework.data.redis.FallbackExceptionTranslationStrategy.translate(FallbackExceptionTranslationStrategy.java:42)
        at org.springframework.data.redis.connection.lettuce.LettuceConnection.convertLettuceAccessException(LettuceConnection.java:272)
        at org.springframework.data.redis.connection.lettuce.LettuceConnection.await(LettuceConnection.java:1063)
        at org.springframework.data.redis.connection.lettuce.LettuceConnection.lambda$doInvoke$4(LettuceConnection.java:920)
        at org.springframework.data.redis.connection.lettuce.LettuceInvoker$Synchronizer.invoke(LettuceInvoker.java:673)
        at org.springframework.data.redis.connection.lettuce.LettuceInvoker$DefaultSingleInvocationSpec.get(LettuceInvoker.java:589)
        at org.springframework.data.redis.connection.lettuce.LettuceGeoCommands.geoRadius(LettuceGeoCommands.java:211)
        at org.springframework.data.redis.connection.DefaultedRedisConnection.geoRadius(DefaultedRedisConnection.java:1520)
        at org.springframework.data.redis.core.DefaultGeoOperations.lambda$radius$7(DefaultGeoOperations.java:185)
        at org.springframework.data.redis.core.RedisTemplate.execute(RedisTemplate.java:223)
        at org.springframework.data.redis.core.RedisTemplate.execute(RedisTemplate.java:190)
        at org.springframework.data.redis.core.AbstractOperations.execute(AbstractOperations.java:97)
        at org.springframework.data.redis.core.DefaultGeoOperations.radius(DefaultGeoOperations.java:185)
        at org.springframework.data.redis.core.DefaultBoundGeoOperations.radius(DefaultBoundGeoOperations.java:144)
        at org.ggcc.commons.utils.RedisGeographyUtil.nearbyListByPosition(RedisGeographyUtil.java:64)
        at org.ggcc.ticket.service.impl.OperatorMiniServiceImpl.getShopInfoListByPosition(OperatorMiniServiceImpl.java:213)
        at org.ggcc.ticket.service.impl.OperatorMiniServiceImpl.getShopTypeList(OperatorMiniServiceImpl.java:490)
        at org.ggcc.ticket.controller.OperatorMiniController.sw$original$getShopTypeList$ua8s811(OperatorMiniController.java:83)
        at org.ggcc.ticket.controller.OperatorMiniController.sw$original$getShopTypeList$ua8s811$accessor$sw$b61kld3(OperatorMiniController.java)
        at org.ggcc.ticket.controller.OperatorMiniController$sw$auxiliary$rign841.call(Unknown Source)
        at org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstMethodsInter.intercept(InstMethodsInter.java:86)

Input Code

Input Code
@ConditionalOnProperty(value = "spring.redis.replica-preferred", havingValue = "true")
@Component
@Slf4j
public class RedisPreferredLettuceClientConfigurationBuilderCustomizer implements LettuceClientConfigurationBuilderCustomizer, InitializingBean {
    @Override
    public void customize(LettuceClientConfiguration.LettuceClientConfigurationBuilder clientConfigurationBuilder) {
        log.info("RedisPreferredLettuceClientConfigurationBuilderCustomizer.customize");
        clientConfigurationBuilder.readFrom(ReadFrom.REPLICA_PREFERRED);
    }

    @Override
    public void afterPropertiesSet() throws Exception {
        log.info("RedisPreferredLettuceClientConfigurationBuilderCustomizer inited");
    }
}


    public List<GeographyDTO<T>> nearbyListByPosition(String key, double longitude, double latitude, int distance, int limit, Class<T> clazz) {
        BoundGeoOperations<String, Object> boundGeoOperations = geoRedisTemple.boundGeoOps(key);
        RedisGeoCommands.GeoRadiusCommandArgs geoRadiusCommandArgs = RedisGeoCommands.GeoRadiusCommandArgs
                .newGeoRadiusArgs().includeDistance().limit(limit).sortAscending();
        List<GeoResult<RedisGeoCommands.GeoLocation<Object>>> geoResultList = Objects.requireNonNull(
                /**
                 *
                 * the under line caused the bug 
                 */
                boundGeoOperations.radius(new Circle(new Point(longitude, latitude), distance), geoRadiusCommandArgs)
        ).getContent();
        if (CollectionUtil.isEmpty(geoResultList)) {
            log.info("缓存中没有推荐数据!");
            return Lists.newArrayList();
        }
        return geoResultList.stream().map(geoResult -> {
            JSONObject jsonObject = JSON.parseObject(JSON.toJSONString(geoResult));
            GeographyDTO<T> geography = new GeographyDTO<>();
            geography.setObject(JSON
                    .parseObject(JSON.toJSONString(JSON.parseObject(JSON.toJSONString(jsonObject.get("content")))
                    .get("name")),clazz));
            geography.setDistance(geoResult.getDistance().getValue());
            return geography;
        }).collect(Collectors.toList());
    }

Expected behavior/code

Environment

  • Lettuce version(s): [6.1.8.RELEASE]. lettuce-core
  • Redis version: [6.2.6]. READ_ONLY NONE

Possible Solution

Additional context

@kennethfan
Copy link
Author

kennethfan commented Apr 18, 2024

io.lettuce.core.masterreplica.ReadOnlyCommands. READ_ONLY_COMMANDS contains geo like command

image
image

@kennethfan
Copy link
Author

redis/redis#4084

GEORADIUS cannot be executed on readonly slave node #4084

@tishun
Copy link
Collaborator

tishun commented Apr 18, 2024

redis/redis#4084

GEORADIUS cannot be executed on readonly slave node #4084

Oh man, what a mess ...

Would using the GEORADIUS_RO work in your case? Do you have a solution to your problem?

@tishun tishun added the status: waiting-for-feedback We need additional information before we can continue label Apr 18, 2024
@kennethfan
Copy link
Author

I create a new class, and use reflection to modify the field ReadOnlyCommands. READ_ONLY_COMMANDS before my custom LettuceClientConfigurationBuilderCustomizer initialize
it works

however, i think is a bug in Lettuce, so i report it as a bug

@kennethfan
Copy link
Author

kennethfan commented Apr 18, 2024

public class ReadOnlyCommandsFix {
    public static void fixRoCommands() throws NoSuchFieldException, IllegalAccessException {
        Class clazz = ReadOnlyCommands.class;
        Field field = clazz.getDeclaredField("READ_ONLY_COMMANDS");
        field.setAccessible(true);
        Set<CommandType> originReadOnlyCommands = (Set<CommandType>) field.get(null);
        Set<CommandType> geoCommands = originReadOnlyCommands.stream()
                .filter(e -> e.name().startsWith("GEO"))
                .collect(Collectors.toSet());
        originReadOnlyCommands.removeAll(geoCommands);
    }
}

this is my way to resolve it , it is ugly

@tishun
Copy link
Collaborator

tishun commented Apr 19, 2024

Can you elaborate more on why you need to remove them from the list in this way?
Is some part of your code depending on iterating over the list?
I assume you can't make a check there?

I agree that the GEORADIUS and GEORADIUSBYMEMBER commands do not belong to this list and we need to remove them. The other GEO* commands in the list should be read-only so they need not be removed.

@tishun tishun added status: good-first-issue An issue that can only be worked on by brand new contributors and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 19, 2024
@kennethfan
Copy link
Author

kennethfan commented Apr 20, 2024

Beacause I have not fund a way to modify or extend the ReadOnlyCommands.isReadOnlyCommand method
So I use a bad way to resolve it
I have not test all the geo like commands, so I remove all the geo like commands, it is not exact

@kennethfan
Copy link
Author

you are right
I agree with you

@kennethfan
Copy link
Author

Perhaps Eval should also be removed

thachlp added a commit to thachlp/lettuce that referenced this issue Jun 4, 2024
@thachlp
Copy link
Contributor

thachlp commented Jun 4, 2024

Hi @tishun, @kennethfan I checked and removed some not read-only commands on the list. Please help review 🙇

@tishun
Copy link
Collaborator

tishun commented Jun 6, 2024

Beacause I have not fund a way to modify or extend the ReadOnlyCommands.isReadOnlyCommand method So I use a bad way to resolve it I have not test all the geo like commands, so I remove all the geo like commands, it is not exact

There is a way to do that, see #2568 (comment)

Perhaps Eval should also be removed

This has been fixed since 6.3.x

tishun added a commit that referenced this issue Jun 22, 2024
* Remove not readonly commands #2832

* Fix comment

Add test

* Update test

* Update cicd.yaml

workaround for apt update issue related to Clearsigned file

* GitHub issue template polishing and stale issues action (#2833)

* GitHub issue template polishing and stale issues action

* Addressing feedback on message test and a better label for the stale issues

* Implement HEXPIRE, HEXPIREAT, HEXPIRETIME and HPERSIST (#2836)

* HEXPIRE implemented with integration tests

* Polishing to integration test, added unit test

* Move new commands to RedisHashCommands, added HEXPIREAT, HEXPIRETIME and HPERSIST

* Make sure we reset the configuration setting after the new hash commands were tested

* Broke one test because of wrong configuration setting; the other is unstable, trying to fix it

* Polishing imports

* Polishin : Copyright change not needed

* MAke sure we wait for the check so it does not fail on the slower pipeline (#2844)

* Add support for `SPUBLISH` (#2838)

* implementation of SPUBLISH

* sort methods by name

* test cluster spublish with no redirects

* use injected cluster client in RedisClusterPubSubConnectionIntegrationTests

* Applying code formatter each time we run a Maven build (#2841)

* Let's try this again

* Add tests and templates

* Merge formatting issues + formatter using wrong configuration

* Update cicd.yaml

the issue related apt repo has been fixed 
https://github.com/orgs/community/discussions/120966#discussioncomment-9211925

* Add support CLIENT KILL [MAXAGE] (#2782)

* Add support CLIENT KILL [MAXAGE]

* polish

* address review changes

* format code

* Add support for `SUNSUBSCRIBE` #2759 (#2851)

* Add support for `SUNSUBSCRIBE` #2759

* replace junit.Assert with assertj

* Mark dnsResolver(DnsResolver) as deprecated. (#2855)

* Implement HPEXPIRE, HPEXPIREAT, HPEXPIRETIME, HTTL and HPTTL (#2857)

* Fixes to the hash field expiration functions

* Implemented HPEXPIRE, HPEXPIREAT, HPEXPIRETIME, HTTL, HPTTL

* Format the files

* FIELDS keyword was introduced as per the PRD

* Extend tests to include HTTL

* Repair unit tests, add new ones for the new commands

* Disable failing test, becasue it is very unstable

* Modify return value to list of long values, fix cluster logic

* Polishing

* Polishing - addressed Ali's comments

* Polishing: Modified by mistake

* Polishing : Addressed Marks' comments

* XREAD support for reading last message from stream (#2863)

* Add last() utility method to the XArgs.StreamOffset

* Submitted one more file by mistake

* Add a `evalReadOnly` overload that accepts the script as a `String` (#2868)

* Add a evalReadOnly overload that accepts the script as a String

* Fix @SInCE annotation for 7.0

* Add release drafter workflow (#2820)

* Release-drafter, dependabot, OCD polishing

* Commitish not needed and pointing to the wrong branch anyway, also wrong version chosen

* Pipeline is also very confusing as there are many pipelines, this is the INTEGRATION pipeline

* Adding commitish, as it is required for filter-by-commitish

* Autolabeling hits issues when using pull_request, using pull_request_target instead

* pull_request_target does nothing, trying the other suggestion from release-drafter/release-drafter#1125

* Bump org.apache.maven.plugins:maven-javadoc-plugin from 3.6.3 to 3.7.0 (#2873)

Bumps [org.apache.maven.plugins:maven-javadoc-plugin](https://github.com/apache/maven-javadoc-plugin) from 3.6.3 to 3.7.0.
- [Release notes](https://github.com/apache/maven-javadoc-plugin/releases)
- [Commits](apache/maven-javadoc-plugin@maven-javadoc-plugin-3.6.3...maven-javadoc-plugin-3.7.0)

---
updated-dependencies:
- dependency-name: org.apache.maven.plugins:maven-javadoc-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump org.codehaus.mojo:flatten-maven-plugin from 1.5.0 to 1.6.0 (#2874)

Bumps [org.codehaus.mojo:flatten-maven-plugin](https://github.com/mojohaus/flatten-maven-plugin) from 1.5.0 to 1.6.0.
- [Release notes](https://github.com/mojohaus/flatten-maven-plugin/releases)
- [Commits](mojohaus/flatten-maven-plugin@1.5.0...1.6.0)

---
updated-dependencies:
- dependency-name: org.codehaus.mojo:flatten-maven-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump org.apache.maven.plugins:maven-jar-plugin from 3.3.0 to 3.4.1 (#2875)

Bumps [org.apache.maven.plugins:maven-jar-plugin](https://github.com/apache/maven-jar-plugin) from 3.3.0 to 3.4.1.
- [Release notes](https://github.com/apache/maven-jar-plugin/releases)
- [Commits](apache/maven-jar-plugin@maven-jar-plugin-3.3.0...maven-jar-plugin-3.4.1)

---
updated-dependencies:
- dependency-name: org.apache.maven.plugins:maven-jar-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump org.openjdk.jmh:jmh-generator-annprocess from 1.21 to 1.37 (#2876)

Bumps [org.openjdk.jmh:jmh-generator-annprocess](https://github.com/openjdk/jmh) from 1.21 to 1.37.
- [Commits](openjdk/jmh@1.21...1.37)

---
updated-dependencies:
- dependency-name: org.openjdk.jmh:jmh-generator-annprocess
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump org.apache.commons:commons-pool2 from 2.11.1 to 2.12.0 (#2877)

Bumps org.apache.commons:commons-pool2 from 2.11.1 to 2.12.0.

---
updated-dependencies:
- dependency-name: org.apache.commons:commons-pool2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Setting the next release to be 6.4.x as part of #2880 (#2881)

* Bump version of lettuce-core to 6.4.0

* Change all @SInCE notations to match the next release 6.4.x and also fixed two typos in the API JavaDoc

* Fixed formatting issues

* Modify the release acrtion to call the proper maven target for release, make releasing manually available too (#2885)

* Format file

* Using interface method from java 8 instead of java 9

* Fix test

* Revert readwrite command to the list

COMMAND INFO READWRITE
1) 1) "readwrite"
   2) "1"
   3) 1) "loading"
      2) "stale"
      3) "fast"

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: atakavci <58048133+atakavci@users.noreply.github.com>
Co-authored-by: Tihomir Krasimirov Mateev <tihomir.mateev@redis.com>
Co-authored-by: Liming Deng <liming.d.pro@gmail.com>
Co-authored-by: Wang Zhi <yfwz100@yeah.net>
Co-authored-by: Luis Miguel Mejía Suárez <BalmungSan@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@tishun tishun closed this as completed Jun 23, 2024
@tishun tishun added this to the 7.x milestone Jun 23, 2024
@kennethfan
Copy link
Author

good job

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
Projects
None yet
Development

No branches or pull requests

3 participants