Remove not readonly commands #2832

[thachlp]

Issue: #2832

Make sure that:

• [x] You have read the contribution guidelines.
• [x] You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
• [x] 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]

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.

[thachlp]

Hey @thachlp , thanks for submitting this PR.

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

1. 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
2. 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"

3. 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]

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

1. 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
2. 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"

3. 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]

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

[thachlp]

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

Thanks @tishun 🙇

[thachlp]

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]

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()](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/List.html#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]

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()](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/List.html#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]

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

[thachlp]

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]

Thanks for the contribution!