Support report libname and libver to Redis #3356

yangbodong22011 · 2023-04-04T09:56:39Z

Reference: redis/redis#11758
Resolves #3338

sazzad16

Error:  testCredentialsProvider(redis.clients.jedis.JedisPooledTest)  Time elapsed: 0.01 s  <<< ERROR!
redis.clients.jedis.exceptions.JedisDataException: NOAUTH Authentication required.
	at redis.clients.jedis.JedisPooledTest.testCredentialsProvider(JedisPooledTest.java:246)

src/main/java/redis/clients/jedis/Connection.java

src/main/java/redis/clients/jedis/args/ClientAttrOption.java

src/main/java/redis/clients/jedis/Connection.java

src/test/java/redis/clients/jedis/JedisPooledTest.java

src/main/java/redis/clients/jedis/args/ClientAttrOption.java

redis#3338 Co-authored-by: M Sazzadul Hoque <[email protected]>

Co-authored-by: M Sazzadul Hoque <[email protected]>

codecov-commenter · 2023-04-07T09:04:33Z

Codecov Report

Patch coverage: 78.84% and project coverage change: +0.03 🎉

Comparison is base (58f5761) 67.11% compared to head (a925493) 67.14%.

❗ Current head a925493 differs from pull request most recent head 1fba4ea. Consider uploading reports for the commit 1fba4ea to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3356      +/-   ##
============================================
+ Coverage     67.11%   67.14%   +0.03%     
- Complexity     4680     4690      +10     
============================================
  Files           266      268       +2     
  Lines         15133    15179      +46     
  Branches        952      959       +7     
============================================
+ Hits          10156    10192      +36     
- Misses         4565     4573       +8     
- Partials        412      414       +2

Impacted Files	Coverage Δ
src/main/java/redis/clients/jedis/Jedis.java	`84.85% <50.00%> (-0.11%)`	⬇️
...n/java/redis/clients/jedis/util/JedisMetaInfo.java	`75.00% <75.00%> (ø)`
src/main/java/redis/clients/jedis/Connection.java	`80.99% <81.81%> (-0.38%)`	⬇️
src/main/java/redis/clients/jedis/Protocol.java	`89.50% <100.00%> (-1.24%)`	⬇️
...edis/clients/jedis/args/ClientAttributeOption.java	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/main/java/redis/clients/jedis/Connection.java

src/test/java/redis/clients/jedis/commands/jedis/ClientCommandsTest.java

Co-authored-by: M Sazzadul Hoque <[email protected]>

yangbodong22011 · 2023-04-07T09:42:35Z

client list output is (4.5.0-SNAPSHOT is my local jar)

id=109 addr=127.0.0.1:64276 laddr=127.0.0.1:6379 fd=10 name= age=26 idle=2 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 qbuf=0 qbuf-free=16890 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=18704 events=r cmd=get user=default redir=-1 resp=2 lib-name=jedis lib-ver=4.5.0-SNAPSHOT

sazzad16 · 2023-04-07T12:04:30Z

src/main/java/redis/clients/jedis/Jedis.java

+  @Override
+  public String clientSetInfo(ClientAttributeOption attr, String value) {
+    checkIsInMultiOrPipeline();
+    connection.sendCommand(CLIENT, SETINFO.getRaw(), attr.getRaw(), encode(value));
+    return connection.getStatusCodeReply();
+  }
+


[Discussion]

@yangbodong22011 WDYT about removing this support from Jedis class? Meaning to keep the usages of SETINFO only internally.

redis/redis#11758 mentions it is for Client libraries, not necessarily for users.

I hope to keep the status quo. Although users do not need to actively set it in most cases, if users want to force it, we can still support it. And redis-cli or sendCommand can also accomplish the same function.

pvarga88 · 2023-05-16T17:47:08Z

Due to this change, Jedis 4.4.0 does not work with pre-7.2 Redis servers.
You seem to have ignored the wisdom from this page:
https://redis.io/commands/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.

sazzad16 · 2023-05-16T18:52:59Z

@pvarga88

Please state which Redis version you are using. It'd be more helpful with the error, stacktrace, etc.

The doc just says ignore failures but doesn't specify those failures. We tried to identify failures as much as we could.

According to my testing, this works fine from Redis 6.2. According to my knowledge, this should work fine from Redis 5.0.
@yangbodong22011 may be able to confirm further.

That leaves us pre-5.0. Frankly, that's not officially supported anymore.

pvarga88 · 2023-05-16T19:01:58Z

Ah, my apologies. This is indeed with a very old Redis server. I didn't expect that would affect Jedis' error handling.

sazzad16 · 2023-05-16T19:08:44Z

@pvarga88 No problem. Sharing information is how we can improve. FYI, there can be a fix coming for Redis 4.0

- fixes library usage for jupiter by applying an disabled ClientSetInfoConfig - see https://redis.io/docs/latest/commands/client-setinfo/ and redis/jedis#3356 (has follow-ups) Fixes: SIRI-921

sazzad16 requested changes Apr 5, 2023

View reviewed changes

src/main/java/redis/clients/jedis/Connection.java Outdated Show resolved Hide resolved

src/main/java/redis/clients/jedis/args/ClientAttrOption.java Outdated Show resolved Hide resolved

src/main/java/redis/clients/jedis/Connection.java Outdated Show resolved Hide resolved

yangbodong22011 force-pushed the feature-client-meta branch from 79b23f9 to 3af2dbd Compare April 6, 2023 07:42

yangbodong22011 commented Apr 6, 2023

View reviewed changes

src/main/java/redis/clients/jedis/Connection.java Outdated Show resolved Hide resolved

yangbodong22011 force-pushed the feature-client-meta branch from 3af2dbd to 29f7c60 Compare April 6, 2023 10:39

sazzad16 reviewed Apr 6, 2023

View reviewed changes

src/test/java/redis/clients/jedis/JedisPooledTest.java Outdated Show resolved Hide resolved

sazzad16 reviewed Apr 6, 2023

View reviewed changes

src/main/java/redis/clients/jedis/args/ClientAttrOption.java Outdated Show resolved Hide resolved

yangbodong22011 and others added 2 commits April 7, 2023 16:52

Support report libname and libver to Redis

406189c

redis#3338 Co-authored-by: M Sazzadul Hoque <[email protected]>

Refactor ClientAttributeOption

b818117

Co-authored-by: M Sazzadul Hoque <[email protected]>

yangbodong22011 force-pushed the feature-client-meta branch from 91ac203 to b818117 Compare April 7, 2023 08:52

redis deleted a comment from codecov-commenter Apr 7, 2023

sazzad16 requested changes Apr 7, 2023

View reviewed changes

sazzad16 and others added 3 commits April 7, 2023 15:06

Apply suggestions from code review

fab5e93

Keep select()

1fba4ea

Update src/main/java/redis/clients/jedis/Connection.java

5c10c75

Co-authored-by: M Sazzadul Hoque <[email protected]>

sazzad16 reviewed Apr 7, 2023

View reviewed changes

sazzad16 approved these changes Apr 7, 2023

View reviewed changes

sazzad16 merged commit 5192937 into redis:master Apr 7, 2023

sazzad16 added this to the 4.4.0 milestone Apr 7, 2023

sazzad16 added the enhancement label Apr 7, 2023

sazzad16 mentioned this pull request Apr 10, 2023

Add support for new redis command CLIENT SETINFO #3342

Closed

sazzad16 added a commit to sazzad16/jedis that referenced this pull request Apr 27, 2023

Address redis#3356

61f42cb

chayim mentioned this pull request May 15, 2023

CI improvements for RESP3 and version validation #3416

Closed

XA21X mentioned this pull request May 25, 2023

Jedis 4.4.1 fails on CLIENT SETINFO for Redis 6.x if the user does not have all +CLIENT permissions #3438

Closed

yangbodong22011 mentioned this pull request Jul 19, 2023

Clients should identify themselves on connect #3482

Closed

mkeckmkeck mentioned this pull request May 27, 2024

Upgrades Jedis library scireum/sirius-db#656

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support report libname and libver to Redis #3356

Support report libname and libver to Redis #3356

yangbodong22011 commented Apr 4, 2023 •

edited by sazzad16

Loading

sazzad16 left a comment

codecov-commenter commented Apr 7, 2023 •

edited

Loading

yangbodong22011 commented Apr 7, 2023 •

edited

Loading

sazzad16 Apr 7, 2023

yangbodong22011 Apr 7, 2023

pvarga88 commented May 16, 2023

sazzad16 commented May 16, 2023

pvarga88 commented May 16, 2023

sazzad16 commented May 16, 2023

Support report libname and libver to Redis #3356

Support report libname and libver to Redis #3356

Conversation

yangbodong22011 commented Apr 4, 2023 • edited by sazzad16 Loading

sazzad16 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 7, 2023 • edited Loading

Codecov Report

yangbodong22011 commented Apr 7, 2023 • edited Loading

sazzad16 Apr 7, 2023

Choose a reason for hiding this comment

yangbodong22011 Apr 7, 2023

Choose a reason for hiding this comment

pvarga88 commented May 16, 2023

sazzad16 commented May 16, 2023

pvarga88 commented May 16, 2023

sazzad16 commented May 16, 2023

yangbodong22011 commented Apr 4, 2023 •

edited by sazzad16

Loading

codecov-commenter commented Apr 7, 2023 •

edited

Loading

yangbodong22011 commented Apr 7, 2023 •

edited

Loading