The KEYS command needs to be keyless

[Dltmd202]

The KEYS command needs to be keyless

The request policy of the KEYS command specifies that the command should have a request_policy:all_shards.

127.0.0.1:6379> COMMAND INFO KEYS
    1)  1) "keys"
    2) (integer) 2
    3) 1) readonly
    4) (integer) 0
    5) (integer) 0
    6) (integer) 0
    7) 1) @keyspace
       2) @read
       3) @slow
       4) @dangerous
    8) 1) "request_policy:all_shards"
       2) "nondeterministic_output_order"
    9) (empty array)
   10) (empty array)

As such the parameters of the command should be considered keyless and the routing should be done to all nodes. Right now we have a K key parameter, which would assume that we need it for routing.

The underlying logic does route (in multi-shard environments) the command to all shards, but it still treats the parameter as a key for the purposes of serialization.

This is somewhat invalid if a user configures their own codec, because it requires that they treat keys and patterns with the same abstraction.

Closes #3311

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 applied code formatting rules using the mvn formatter:format target. Don’t submit any formatting related changes.
• You submit test cases (unit or integration tests) that back your changes.

[Dltmd202]

Hi @tishun, could you please take a look at this PR when you have a moment?

[tishun]

Hey @Dltmd202 ,

unfortunately we can't change the existing API without deprecating it first. So can you please add new methods with the new signature, and leave out the old ones with a @deprecated notice in the JAvaDoc and a @deprecated annotation of each API method?

[Dltmd202]

Hi @tishun,

Got it — I’ll go ahead and add the new methods with the updated signature, and mark the existing ones as deprecated using the @deprecated annotation and JavaDoc. Thanks for the clarification!

[Dltmd202]

Hi, @tishun

While working on a contribution, I ran into a problem with the keys methods.
We currently have

List<K> keys(String pattern);      // new API
@Deprecated
List<K> keys(K pattern);           // legacy API

When K is String, both look like keys(String) to the compiler, so it can’t decide which one to call.

Example from a test:

@Test
void testComposedCodec() {
    RedisCodec<String, Object> composed =
        RedisCodec.of(StringCodec.ASCII, new SerializedObjectCodec());
    RedisCommands<String, Object> connection = client.connect(composed).sync();

    connection.set(key, new Person());

    // Ambiguous when K = String
    List<String> keys = connection.keys(key);

    assertThat(keys).hasSize(1);
    assertThat(connection.get(key)).isInstanceOf(Person.class);
    connection.getStatefulConnection().close();
}

This makes it tricky to keep both signatures during a deprecation phase.
As far as I know, Java can’t express “K must not be String” here, so the only safe options seem to be:
• Rename the legacy method during deprecation, then remove it later.
• Move one of them to a separate interface so they don’t clash.

Let me know which direction you’d like to take, and I can adjust the PR accordingly.

[tishun]

Ah, unfortunate, indeed.

IMHO an exception to the rule can be made here, since we are considering to release 7.x, which is a major release:

• use the old signature for the right type keys(String pattern)
• use the new signature for the wrong type keysLegacy(K pattern)
• deprecate keysLegacy(K pattern) with the @Deprecated annotation
• add JavaDoc to both explaining that the keysLegacy will be removed in later versions

Argumentation:

• although we will break the contract without deprecating first we will still provide a fallback mechanism
• users of the right type would not have to make changes in the future (if we used another name)

If we were to follow a different route then we would either have to change the API a second time (when we finally remove the deprecated API); which - by the same rule - would require first to deprecate the newly introduced method and then to remove it in a separate release; Eventually having to keep this mess for another major release.

@ggivo , @a-TODO-rov , @uglide - what do you think?

[a-TODO-rov]

What are the advantages of adding new GlobalPatternArgument against using StringArgument ?

[github-actions]

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

[a-TODO-rov]

Hey @Dltmd202 thank you for your contribution. I finished the implementation as @tishun suggested.

[a-TODO-rov]

Why do we need different output ?

[a-TODO-rov]

Useless line in legacy impl.

[a-TODO-rov]

Useless line in legacy impl

[tishun]

LGTM

[Dltmd202]

I appreciate you catching that. I overlooked it during my review — thanks for taking care of it! @a-TODO-rov