-
Notifications
You must be signed in to change notification settings - Fork 91
feat: add cache factory, unify client interface, remove fallback (#4558) #4623
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #4623 +/- ##
==========================================
- Coverage 95.48% 86.95% -8.54%
==========================================
Files 129 131 +2
Lines 20903 20938 +35
Branches 1782 1482 -300
==========================================
- Hits 19959 18206 -1753
- Misses 924 2682 +1758
- Partials 20 50 +30
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 44 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@jasuwienas Nice work it's a big one! However, The issue #4558 seems to ask for a fairly simple change: adding a I’m also seeing things like MeasureCache and FallbackCache, which look like new concepts. Have we had a design session as a team about these changes? I’m just wondering if we’re adding more complexity than needed for what was supposed to be a straightforward technical-debt fix. |
|
@quiet-node Right, I'll prepare an ADR.
Not exactly. Our current implementation works like this:
This is precisely what the FallbackClient was originally designed to support. With that approach, the factory would still return one unified client object with a consistent interface. The alternative would be this: https://github.com/hiero-ledger/hiero-json-rpc-relay/pull/4628/files the factory returns both the shared and internal clients here, allowing us to keep cacheService as it is. Or this: (we are calling the factory twice) |
b9a177e to
57dc361
Compare
|
Dropping the fallback mechanism is a breaking change! During review, please take a look at: |
|
BTW. To investigate the retry strategy take a look at packages/relay/src/lib/client/redisClientManager.ts I think we already have it covered there. |
5ca2b03 to
6df6f50
Compare
…ro-ledger#4558) Signed-off-by: Mariusz Jasuwienas <[email protected]>
6df6f50 to
28e225c
Compare
Signed-off-by: Mariusz Jasuwienas <[email protected]>
Signed-off-by: Mariusz Jasuwienas <[email protected]>
Description
Related issue(s)
Fixes #4558
Testing Guide
Changes from original design (optional)
Additional work needed
The Redis cache service still implements an additional, unnecessary interface and only measures logs and delegates calls to the underlying client. It will be removed entirely in this PR.
Checklist