Remove not readonly commands #2832 #2871

thachlp · 2024-06-04T14:28:26Z

Make sure that:

You have read the contribution guidelines.
You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.

tishun

Hey @thachlp , thanks for submitting this PR.

I think there are a few things we needs to address first:

Seems like there are two ReadOnlyCommands classes - one in the io.lettuce.core.masterreplica package and one in the io.lettuce.core.protocol package. The one you modified (in the io.lettuce.core.masterreplica) appears to be unused. I believe the change needs to be done in the io.lettuce.core.protocol.ReadOnlyCommands class. Let's change both classes for now similar to how it was done in Add GEOSEARCH to read-only commands #2568 #2569
The GEOSEARCH command is read-only, let's not remove it from the list:

COMMAND INFO GEOSEARCH
1) 1) "geosearch"
   2) "-7"
   3) 1) "readonly"
   4) "1"

In a perfect world I'd love to see an integration test that loops over these commands and verifies that they are readonly so we can quickly adapt to server change (optional, but would be very nice).

tishun · 2024-06-06T14:02:26Z

Just talked to @mp911de and he agreed that we should probably remove io.lettuce.core.masterreplica. ReadOnlyCommands. This could be done in another PR, but it would be awesome if we can do it here.

Add test

thachlp · 2024-06-08T05:43:45Z

Hey @thachlp , thanks for submitting this PR.

I think there are a few things we needs to address first:

Seems like there are two ReadOnlyCommands classes - one in the io.lettuce.core.masterreplica package and one in the io.lettuce.core.protocol package. The one you modified (in the io.lettuce.core.masterreplica) appears to be unused. I believe the change needs to be done in the io.lettuce.core.protocol.ReadOnlyCommands class. Let's change both classes for now similar to how it was done in Add GEOSEARCH to read-only commands #2568 #2569

The GEOSEARCH command is read-only, let's not remove it from the list:
COMMAND INFO GEOSEARCH
1) 1) "geosearch"
   2) "-7"
   3) 1) "readonly"
   4) "1"
In a perfect world I'd love to see an integration test that loops over these commands and verifies that they are readonly so we can quickly adapt to server change (optional, but would be very nice).

@tishun,
You are correct, I rechecked and updated the list, please help review 🚀
Is there any way that I can run the integration test in local? I try to write the test that you recommend but I got the error
java: package jdk.net does not exist. How can I fix it?

tishun · 2024-06-08T06:14:03Z

Hey @thachlp , thanks for submitting this PR.
I think there are a few things we needs to address first:

Seems like there are two ReadOnlyCommands classes - one in the io.lettuce.core.masterreplica package and one in the io.lettuce.core.protocol package. The one you modified (in the io.lettuce.core.masterreplica) appears to be unused. I believe the change needs to be done in the io.lettuce.core.protocol.ReadOnlyCommands class. Let's change both classes for now similar to how it was done in Add GEOSEARCH to read-only commands #2568 #2569

The GEOSEARCH command is read-only, let's not remove it from the list:
COMMAND INFO GEOSEARCH
1) 1) "geosearch"
   2) "-7"
   3) 1) "readonly"
   4) "1"
In a perfect world I'd love to see an integration test that loops over these commands and verifies that they are readonly so we can quickly adapt to server change (optional, but would be very nice).
@tishun, You are correct, I rechecked and updated the list, please help review 🚀 Is there any way that I can run the integration test in local? I try to write the test that you recommend but I got the error java: package jdk.net does not exist. How can I fix it?

You should be able to run test locally if you call make test

tishun

Looks perfect, the two notes are good-to-have

tishun · 2024-06-10T07:23:46Z

src/test/java/io/lettuce/core/commands/ReadOnlyCommandIntegrationTests.java

+    }
+
+    @Test
+    void testReadOnlyCommands() {


Awesome! This is exactly what I wanted to see and it would help us keep this list up-to-date.

Two notes:

you can move this test to the ServerCommandIntegrationTest - no need to create a whole new class for it

you can use the CommandDetailParser to parse the result, instead of parsing it yourself (would also mean that if something changes in the result API you would not have to modify the test and we will keep the change locally to the parser)

Hi @tishun, thanks for the notes.
I updated the test, please help check 🙇

btw, If anything need to pass the CI check, please suggest.
On local, I got this message:

[ERROR] Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.16.0:validate (default) on project lettuce-core: File '/...lettuce/src/test/java/io/lettuce/core/commands/ServerCommandIntegrationTests.java' has not been previously formatted. Please format file and commit before running validation! -> [Help 1]

tishun · 2024-06-10T16:07:43Z

You'd need to format your new changes too, try using mvn formatter:format

thachlp · 2024-06-11T02:37:09Z

You'd need to format your new changes too, try using mvn formatter:format

Thanks @tishun 🙇

thachlp · 2024-06-11T14:24:27Z

You'd need to format your new changes too, try using mvn formatter:format

Thanks @tishun 🙇

Hi @tishun, the error from CI is weird, could you re-run the job? Do you have any recommend?

tishun · 2024-06-12T06:36:54Z

You'd need to format your new changes too, try using mvn formatter:format

Thanks @tishun 🙇

Hi @tishun, the error from CI is weird, could you re-run the job? Do you have any recommend?

Hey @thachlp , List.of() is a feature of Java 9, and we compile with Java 8 to achieve maximum backwards compatibility.

Could you change the use of this method with something else like Arrays.asList()

thachlp · 2024-06-12T08:37:26Z

You'd need to format your new changes too, try using mvn formatter:format

Thanks @tishun 🙇

Hi @tishun, the error from CI is weird, could you re-run the job? Do you have any recommend?

Hey @thachlp , List.of() is a feature of Java 9, and we compile with Java 8 to achieve maximum backwards compatibility.

Could you change the use of this method with something else like Arrays.asList()

Nice, thanks

tishun · 2024-06-21T12:20:48Z

I think you need to rebase your changes to include the latest fixes to the unit tests to pass the CI

workaround for apt update issue related to Clearsigned file

* GitHub issue template polishing and stale issues action * Addressing feedback on message test and a better label for the stale issues

* 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

…eline (redis#2844)

* implementation of SPUBLISH * sort methods by name * test cluster spublish with no redirects * use injected cluster client in RedisClusterPubSubConnectionIntegrationTests

* Let's try this again * Add tests and templates * Merge formatting issues + formatter using wrong configuration

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

* Add support CLIENT KILL [MAXAGE] * polish * address review changes * format code

* Add support for `SUNSUBSCRIBE` redis#2759 * replace junit.Assert with assertj

* Add last() utility method to the XArgs.StreamOffset * Submitted one more file by mistake

@SInCE

…edis#2868) * Add a evalReadOnly overload that accepts the script as a String * Fix @SInCE annotation for 7.0

* 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

redis#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>

…s#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>

…edis#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>

…s#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>

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>

@SInCE

* 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

…e, make releasing manually available too (redis#2885)

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

thachlp · 2024-06-22T05:11:57Z

I think you need to rebase your changes to include the latest fixes to the unit tests to pass the CI

@tishun, it's done 🙇

tishun · 2024-06-22T09:36:00Z

Thanks for the contribution!

…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

…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

Remove not readonly commands redis#2832

10ebbf4

tishun requested changes Jun 6, 2024

View reviewed changes

tishun added this to the 7.0.0.RELEASE milestone Jun 6, 2024

tishun added the type: bug A general bug label Jun 6, 2024

tishun modified the milestones: 7.0.0.RELEASE, 7.x Jun 6, 2024

Fix comment

e3080fd

Add test

thachlp requested a review from tishun June 8, 2024 05:43

tishun reviewed Jun 10, 2024

View reviewed changes

tishun approved these changes Jun 10, 2024

View reviewed changes

Update test

36dc55a

atakavci and others added 10 commits June 22, 2024 11:51

Update cicd.yaml

1dcef97

workaround for apt update issue related to Clearsigned file

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

430945e

* GitHub issue template polishing and stale issues action * Addressing feedback on message test and a better label for the stale issues

MAke sure we wait for the check so it does not fail on the slower pip…

9781769

…eline (redis#2844)

Add support for SPUBLISH (redis#2838)

53050ea

* 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 (redis#2841)

e8ae914

* Let's try this again * Add tests and templates * Merge formatting issues + formatter using wrong configuration

Update cicd.yaml

c0c5782

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

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

4eac609

* Add support CLIENT KILL [MAXAGE] * polish * address review changes * format code

Add support for SUNSUBSCRIBE redis#2759 (redis#2851)

2899770

* Add support for `SUNSUBSCRIBE` redis#2759 * replace junit.Assert with assertj

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

7a5f988

tishun and others added 14 commits June 22, 2024 11:52

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

5612e29

* 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 (r…

7d5087e

…edis#2868) * Add a evalReadOnly overload that accepts the script as a String * Fix @SInCE annotation for 7.0

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

544ec67

* 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 releas…

caadbfe

…e, make releasing manually available too (redis#2885)

Format file

74ec22f

Using interface method from java 8 instead of java 9

8fdbd97

Fix test

a1ab7ef

Revert readwrite command to the list

fe2f247

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

thachlp force-pushed the remove-not-read-only-command branch from 1cdc708 to fe2f247 Compare June 22, 2024 04:53

Merge branch 'main' into remove-not-read-only-command

2f54211

tishun merged commit d507743 into redis:main Jun 22, 2024
5 checks passed

thachlp deleted the remove-not-read-only-command branch June 22, 2024 13:32

tishun mentioned this pull request Jun 23, 2024

[#2341] Initialize slots with empty BitSet in RedisClusterNode's constructors #2852

Merged

4 tasks

tishun modified the milestones: 7.x, 6.5.0.RELEASE, 6.4.0.RELEASE Aug 16, 2024

ggivo mentioned this pull request Oct 30, 2024

OpsForGeo producing "READONLY You can't write against a read only replica " on READS... #3032

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove not readonly commands #2832 #2871

Remove not readonly commands #2832 #2871

thachlp commented Jun 4, 2024 •

edited by tishun

Loading

tishun left a comment

tishun commented Jun 6, 2024

thachlp commented Jun 8, 2024

tishun commented Jun 8, 2024

tishun left a comment

tishun Jun 10, 2024

thachlp Jun 10, 2024

thachlp Jun 10, 2024

tishun commented Jun 10, 2024

thachlp commented Jun 11, 2024

thachlp commented Jun 11, 2024

tishun commented Jun 12, 2024

thachlp commented Jun 12, 2024

tishun commented Jun 21, 2024

thachlp commented Jun 22, 2024

tishun commented Jun 22, 2024

Remove not readonly commands #2832 #2871

Remove not readonly commands #2832 #2871

Conversation

thachlp commented Jun 4, 2024 • edited by tishun Loading

tishun left a comment

Choose a reason for hiding this comment

tishun commented Jun 6, 2024

thachlp commented Jun 8, 2024

tishun commented Jun 8, 2024

tishun left a comment

Choose a reason for hiding this comment

tishun Jun 10, 2024

Choose a reason for hiding this comment

thachlp Jun 10, 2024

Choose a reason for hiding this comment

thachlp Jun 10, 2024

Choose a reason for hiding this comment

tishun commented Jun 10, 2024

thachlp commented Jun 11, 2024

thachlp commented Jun 11, 2024

tishun commented Jun 12, 2024

thachlp commented Jun 12, 2024

tishun commented Jun 21, 2024

thachlp commented Jun 22, 2024

tishun commented Jun 22, 2024

thachlp commented Jun 4, 2024 •

edited by tishun

Loading