-
Notifications
You must be signed in to change notification settings - Fork 93
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
|
@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: https://github.com/jasuwienas/hiero-json-rpc-relay/tree/4558-factory-for-cache-client-simplified-2 (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 <jasuwienas@gmail.com>
6df6f50 to
28e225c
Compare
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Signed-off-by: jasuwienas <jasuwienas@gmail.com>
…dger#4558) Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (68.38%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.
@@ Coverage Diff @@
## main #4623 +/- ##
===========================================
- Coverage 95.47% 68.38% -27.09%
===========================================
Files 129 131 +2
Lines 20928 21019 +91
Branches 1795 519 -1276
===========================================
- Hits 19981 14374 -5607
- Misses 927 6631 +5704
+ Partials 20 14 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 72 files with indirect coverage changes 🚀 New features to boost your workflow:
|
…ro-ledger#4558) Signed-off-by: Mariusz Jasuwienas <jasuwienas@gmail.com>
cadb459 to
3ebaeaa
Compare
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