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

fix: HotFix for using mSet instead of Set for redis cache when storing synthetic transactions #2197

Merged
merged 5 commits into from
Mar 14, 2024

Conversation

AlfredoG87
Copy link
Collaborator

Description:

  • Added method interface mSet for ICacheClient
  • Implemented method on both cache clients: Redis and Local
  • Added method mSet to cache service
  • Refactored method filterAndPopulateSyntheticContractResults to use mSet instead of set.

Related issue(s):

Fixes #

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@AlfredoG87 AlfredoG87 changed the title Fix to use mSet instead of set, so we can batch the requests to the r… fix: HotFix for using mSet instead of Set for redis cache when storing synthetic transactions Mar 14, 2024
Copy link

github-actions bot commented Mar 14, 2024

Tests

    1 files    35 suites   3s ⏱️
194 tests 193 ✔️ 1 💤 0
197 runs  196 ✔️ 1 💤 0

Results for commit 77f57ec.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Mar 14, 2024

Acceptance Tests

  12 files  148 suites   15m 23s ⏱️
393 tests 389 ✔️ 4 💤 0
414 runs  410 ✔️ 4 💤 0

Results for commit 77f57ec.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

A few suggestions.
Also please update the description with context for the socket optimization.

packages/relay/src/lib/clients/cache/ICacheClient.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/clients/cache/redisCache.ts Outdated Show resolved Hide resolved
Signed-off-by: Alfredo Gutierrez <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.15%. Comparing base (6acced8) to head (1dbad1a).

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

Additional details and impacted files
@@              Coverage Diff              @@
##           release/0.43    #2197   +/-   ##
=============================================
  Coverage         75.15%   75.15%           
=============================================
  Files                13       13           
  Lines               644      644           
  Branches            118      118           
=============================================
  Hits                484      484           
  Misses              115      115           
  Partials             45       45           

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

Signed-off-by: Alfredo Gutierrez <[email protected]>
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

2 suggestions.

Are there any additional tests we can add, especially anything to capture the redis process state changes

…to the end so we dont capture and then fail (if it fails)

Signed-off-by: Alfredo Gutierrez <[email protected]>
@AlfredoG87
Copy link
Collaborator Author

@Nana-EC I've added unit tests that verify that multiSet is working as expected and that verifies the cache state changes

@AlfredoG87 AlfredoG87 self-assigned this Mar 14, 2024
Copy link

sonarcloud bot commented Mar 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@AlfredoG87 AlfredoG87 added bug Something isn't working enhancement New feature or request and removed enhancement New feature or request labels Mar 14, 2024
@AlfredoG87 AlfredoG87 modified the milestones: 0.44.0, 0.43.0, 0.43.1 Mar 14, 2024
@AlfredoG87 AlfredoG87 requested a review from Nana-EC March 14, 2024 21:55
@AlfredoG87 AlfredoG87 marked this pull request as ready for review March 14, 2024 21:56
@AlfredoG87 AlfredoG87 requested review from ebadiere, lukelee-sl and a team as code owners March 14, 2024 21:56
@ebadiere ebadiere merged commit e196bcd into release/0.43 Mar 14, 2024
27 of 28 checks passed
@ebadiere ebadiere deleted the hot-fix-mset-redis-cache branch March 14, 2024 22:29
AlfredoG87 added a commit that referenced this pull request Mar 18, 2024
…g synthetic transactions (#2197)

* Fix to use mSet instead of set, so we can batch the requests to the redis cache

Signed-off-by: Alfredo Gutierrez <[email protected]>

* Feedback of PR Review

Signed-off-by: Alfredo Gutierrez <[email protected]>

* some improvements

Signed-off-by: Alfredo Gutierrez <[email protected]>

* add missing metric observations and leaving the metrics observations to the end so we dont capture and then fail (if it fails)

Signed-off-by: Alfredo Gutierrez <[email protected]>

* adding unit tests for MultiSet state changes on the cache

Signed-off-by: Alfredo Gutierrez <[email protected]>

---------

Signed-off-by: Alfredo Gutierrez <[email protected]>
AlfredoG87 added a commit that referenced this pull request Mar 18, 2024
fix: HotFix for using mSet instead of Set for redis cache when storing synthetic transactions (#2197)

* Fix to use mSet instead of set, so we can batch the requests to the redis cache



* Feedback of PR Review



* some improvements



* add missing metric observations and leaving the metrics observations to the end so we dont capture and then fail (if it fails)



* adding unit tests for MultiSet state changes on the cache



---------

Signed-off-by: Alfredo Gutierrez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants