Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid using QUIT command #3353

Merged
merged 3 commits into from
Apr 6, 2023

Conversation

yangbodong22011
Copy link
Collaborator

@yangbodong22011 yangbodong22011 commented Apr 3, 2023

Please refer to: redis/redis#11420

This PR cancels calling the quit command when the Jedis internal link is abnormal, but the user interface Jedis.quit() is not affected. This optimizes Jedis connection pool performance and addresses #2105 and #2108

See comments for some code details.

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (995f5fe) 67.02% compared to head (c8bb28b) 67.07%.

❗ Current head c8bb28b differs from pull request most recent head c503aff. Consider uploading reports for the commit c503aff 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    #3353      +/-   ##
============================================
+ Coverage     67.02%   67.07%   +0.04%     
+ Complexity     4661     4657       -4     
============================================
  Files           263      263              
  Lines         15101    15072      -29     
  Branches        952      947       -5     
============================================
- Hits          10122    10109      -13     
+ Misses         4570     4556      -14     
+ Partials        409      407       -2     
Impacted Files Coverage Δ
src/main/java/redis/clients/jedis/Jedis.java 84.95% <ø> (ø)
src/main/java/redis/clients/jedis/Connection.java 81.37% <100.00%> (-1.56%) ⬇️
...in/java/redis/clients/jedis/ConnectionFactory.java 80.00% <100.00%> (+12.65%) ⬆️
...rc/main/java/redis/clients/jedis/JedisFactory.java 70.65% <100.00%> (+6.50%) ⬆️
src/main/java/redis/clients/jedis/Protocol.java 90.74% <100.00%> (+1.23%) ⬆️

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

Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both Connection and Jedis has quit() methods. Deprecate those and suggest users to use disconnect() in javadoc.

@sazzad16 sazzad16 requested a review from dengliming April 3, 2023 11:38
@sazzad16 sazzad16 added this to the 4.4.0 milestone Apr 3, 2023
Please refer to: redis/redis#11420

This PR cancels calling the quit command when the Jedis internal
link is abnormal, and deprecate the user interface `Jedis.quit()`.
This optimizes Jedis connection pool performance and
fixes: redis#2105
redis#2108
@yangbodong22011
Copy link
Collaborator Author

Both Connection and Jedis has quit() methods. Deprecate those and suggest users to use disconnect() in javadoc.

done

@yangbodong22011 yangbodong22011 changed the title Jedis (inner) avoid using QUIT command Jedis avoid using QUIT command Apr 4, 2023
@sazzad16 sazzad16 changed the title Jedis avoid using QUIT command Avoid using QUIT command Apr 5, 2023
dengliming
dengliming previously approved these changes Apr 6, 2023
Co-authored-by: M Sazzadul Hoque <7600764+sazzad16@users.noreply.github.com>
Co-authored-by: M Sazzadul Hoque <7600764+sazzad16@users.noreply.github.com>
@sazzad16 sazzad16 merged commit 3e8a584 into redis:master Apr 6, 2023
sazzad16 pushed a commit to sazzad16/jedis that referenced this pull request Apr 20, 2023
* Jedis avoid using QUIT command

Please refer to: redis/redis#11420

This PR cancels calling the quit command when the Jedis internal
link is abnormal, and deprecate the user interface `Jedis.quit()`.
This optimizes Jedis connection pool performance and
fixes: redis#2105

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

Co-authored-by: M Sazzadul Hoque <7600764+sazzad16@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: M Sazzadul Hoque <7600764+sazzad16@users.noreply.github.com>

---------

Co-authored-by: M Sazzadul Hoque <7600764+sazzad16@users.noreply.github.com>

---------

 Conflicts:
	src/main/java/redis/clients/jedis/Connection.java
	src/main/java/redis/clients/jedis/ConnectionFactory.java
	src/main/java/redis/clients/jedis/Jedis.java
	src/main/java/redis/clients/jedis/Protocol.java
sazzad16 added a commit that referenced this pull request Apr 20, 2023
Backport of #3353 

---------

Please refer to: redis/redis#11420

This PR cancels calling the quit command when the Jedis internal link is abnormal, and deprecate the user interface `Jedis.quit()`. This optimizes Jedis connection pool performance and fixes: #2105

---------

 Conflicts:
	src/main/java/redis/clients/jedis/Connection.java
	src/main/java/redis/clients/jedis/ConnectionFactory.java
	src/main/java/redis/clients/jedis/Jedis.java
	src/main/java/redis/clients/jedis/Protocol.java

* more changes

* Modify makeObject() for Jedis 3.x

---------

Co-authored-by: bodong.ybd <bodong.ybd@alibaba-inc.com>
sazzad16 added a commit to sazzad16/jedis that referenced this pull request Apr 27, 2023
sazzad16 added a commit to sazzad16/jedis that referenced this pull request Apr 27, 2023
Address 'Address+Fix redis#3353'
sazzad16 added a commit to sazzad16/jedis that referenced this pull request Apr 27, 2023
sazzad16 added a commit to sazzad16/jedis that referenced this pull request Apr 27, 2023
Address 'Address+Fix redis#3353'
sazzad16 added a commit that referenced this pull request Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants