Send the CLIENT SETINFO command in a fire-and-forget way

[tishun]

Closes #2817

This solution aims to bring consistency between the Lettuce driver and most other drivers maintained by the Redis team.
The current approach is - instead of probing for the version of the remote server and deciding based on that wether or not to send the CLIENT SETINFO command - to always send it and ignore the server response. There are two main reasons behind this: different flavors of the Redis server handle this in different versions and also the information sent is not critical to the operation of the driver, so failures should not necessary terminate the connection.

1. Tested against Redis 6.0 - verified no connection failures are shown
2. Tested against Redis (unstable) - verified no connection failures are shown; and that CLIENT LIST contains the expected data on the lib-name and lib-ver are set properly.
3. Added unit tests

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.

[mp911de]

to be fair, the method name doesn't reflect what is happening here as lenient error handling happens in the calling code.

Looking at the code, it might make sense to introduce stronger guards to avoid ArrayList creation if neither library name nor library version are set and so avoid also the call to dispatch.

Just a couple of thoughts.

[tishun]

Fair assessment :) Was a bit hasty with the change, will update with your suggestions. Thanks!

[tishun]

That being said, @mp911de , would you say that we can also move the client name part to the more-lenient second post handshake too?

        if (metadata.getClientName() != null && negotiatedProtocolVersion == ProtocolVersion.RESP2) {
            postHandshake.add(new AsyncCommand<>(this.commandBuilder.clientSetname(connectionState.getClientName())));
        }

[mp911de]

TBH, I'm not sure about fire+forget.

There are quite some commands being invoked on startup and any failures can go unnoticed. Someone relying on the client name who has renamed the command doesn't find out about a failure until they investigate the client name on their own.

If someone encounters failures because of these commands, then they should not set the client name, library version, ….

What is the rationale to ignore errors?

[tishun]

Could you give me an example where this metadata is critical to the operation of the client?
So far I assumed this is mostly tracing information. Are there flows where one client expects to find another one based on the name the other client provided? How would they handle this in older versions in Redis? I would be very hesitant to endorse the usage of these fields in this way

Even so I think the same rationale does not exist for lib-name and lib-version - these are definitely tracing information that is secondary to the operation of the driver.

Here is an excerpt from the documentation for the CLIENT SETINFO:

Client libraries are expected to pipeline this command after authentication on all connections and ignore failures since they could be connected to an older version that doesn't support them.

As I mentioned different flavors of the Redis server handle this in different versions and to embed a support matrix in the client would be counterproductive when the operation can simply fail. #2817 is a testament to this issue.

Would you consider it an improvement if we log the issue?

[uglide]

@mp911de I am just adding my 2 cents here. Command renaming is seldom used nowadays because Redis provides more efficient ways to secure the connection, etc.

[mp911de]

Probably logging at the debug level is a good common ground. That being said, CLIENT SETNAME belongs into the same category.

[tishun]

Fair, let me prepare another change. Thanks for considering this!

[tishun]

@mp911de what about this version?

[codecov]

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 77.73%. Comparing base (43843bf) to head (5dffa53).
Report is 249 commits behind head on main.

❗ Current head 5dffa53 differs from pull request most recent head 9e9483a. Consider uploading reports for the commit 9e9483a to get more accurate results

Files	Patch %	Lines
src/main/java/io/lettuce/core/RedisHandshake.java	90.47%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2823      +/-   ##
============================================
- Coverage     78.71%   77.73%   -0.98%     
- Complexity     6786     7241     +455     
============================================
  Files           508      539      +31     
  Lines         22765    24491    +1726     
  Branches       2446     2607     +161     
============================================
+ Hits          17919    19039    +1120     
- Misses         3717     4255     +538     
- Partials       1129     1197      +68     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.