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

♻️ refactor RedisClientsManager and RedisClientSDK #5888

Merged

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented May 29, 2024

What do these changes do?

  • ✨ added decode_responses to RedisClientSDK
  • RedisClientsManager initialisation changed to allow for overwriting of default values used in the constructor of RedisClientSDK
  • ♻️ RedisClientSDK clients now expose the health of the connection to Redis.
  • ⚰️ removed HealthCheckedRedisClientSDK and merged with RedisClientSDK

Related issue/s

How to test

Dev-ops checklist

@GitHK GitHK self-assigned this May 29, 2024
@GitHK GitHK added the a:services-library issues on packages/service-libs label May 29, 2024
@GitHK GitHK added this to the Leeroy Jenkins milestone May 29, 2024
@GitHK GitHK marked this pull request as ready for review May 29, 2024 06:49
@GitHK GitHK requested a review from pcrespov as a code owner May 29, 2024 06:49
Copy link

codecov bot commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.5%. Comparing base (cafbf96) to head (12c91e6).
Report is 265 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5888      +/-   ##
=========================================
+ Coverage    84.5%   87.5%    +2.9%     
=========================================
  Files          10    1110    +1100     
  Lines         214   48293   +48079     
  Branches       25     716     +691     
=========================================
+ Hits          181   42261   +42080     
- Misses         23    5872    +5849     
- Partials       10     160     +150     
Flag Coverage Δ
integrationtests 65.3% <100.0%> (?)
unittests 85.2% <100.0%> (+0.6%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...src/servicelib/deferred_tasks/_deferred_manager.py 89.8% <100.0%> (ø)
...c/servicelib/deferred_tasks/_redis_task_tracker.py 100.0% <100.0%> (ø)
packages/service-library/src/servicelib/redis.py 95.2% <100.0%> (ø)
...s/service-library/src/servicelib/retry_policies.py 100.0% <100.0%> (ø)
...2/src/simcore_service_director_v2/modules/redis.py 100.0% <100.0%> (ø)
...ervice_dynamic_scheduler/api/rest/_dependencies.py 100.0% <100.0%> (ø)
...core_service_dynamic_scheduler/api/rest/_health.py 100.0% <100.0%> (ø)
...imcore_service_dynamic_scheduler/services/redis.py 100.0% <100.0%> (ø)
.../web/server/src/simcore_service_webserver/redis.py 95.6% <100.0%> (ø)

... and 1099 files with indirect coverage changes

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

please check @pcrespov comments.

packages/service-library/src/servicelib/redis.py Outdated Show resolved Hide resolved
@GitHK GitHK changed the title ♻️ RedisClientsManager uses only health check clients ♻️ refactor RedisClientsManager Jun 4, 2024
@GitHK GitHK requested review from pcrespov and sanderegg June 4, 2024 09:41
@GitHK
Copy link
Contributor Author

GitHK commented Jun 4, 2024

@sanderegg @pcrespov I had to rework this PR a bit, please read the updated description again

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

some early comments

packages/service-library/src/servicelib/redis.py Outdated Show resolved Hide resolved
packages/service-library/src/servicelib/redis.py Outdated Show resolved Hide resolved
packages/service-library/src/servicelib/redis.py Outdated Show resolved Hide resolved
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

thx. If some of the feedback is unclear, please let me know if you prefer to discuss offline

services/web/server/src/simcore_service_webserver/redis.py Outdated Show resolved Hide resolved
packages/service-library/src/servicelib/redis.py Outdated Show resolved Hide resolved
packages/service-library/src/servicelib/redis.py Outdated Show resolved Hide resolved
packages/service-library/tests/test_redis.py Outdated Show resolved Hide resolved
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

added some more comments. I don't get why your new class needs a init function since it is a dataclass.
Also, I would prefer that we don't have 2 different clients but just 1. is it not possible to migrate the dynamic sidecar one, and just keep one simple client?

packages/service-library/src/servicelib/redis.py Outdated Show resolved Hide resolved
packages/service-library/src/servicelib/redis.py Outdated Show resolved Hide resolved
@GitHK GitHK changed the title ♻️ refactor RedisClientsManager ♻️ refactor RedisClientsManager and RedisClientSDK Jun 11, 2024
@GitHK GitHK requested a review from sanderegg June 11, 2024 08:48
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

thank you!

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.1% Duplication on New Code

See analysis details on SonarCloud

@mrnicegyu11
Copy link
Member

@GitHK requested a force merge

@mrnicegyu11 mrnicegyu11 merged commit 641b328 into ITISFoundation:master Jun 12, 2024
57 checks passed
@GitHK GitHK deleted the redis-clients-manager-health-checked branch June 12, 2024 13:23
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jul 5, 2024
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:services-library issues on packages/service-libs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants