Implement composite cache#765
Conversation
|
👋 mxiao-cll, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
NPM Publishing labels 🏷️🟢 This PR has valid version labels and will cause a |
alejoberardino
left a comment
There was a problem hiding this comment.
@mxiao-cll from the ticket for this change I agree with your challenge arguments wholeheartedly, would be good to see the intended effect of this change in the concrete impl you have in mind even as a draft
Big PR here you go and example usage here: https://github.com/smartcontractkit/external-adapters-js/pull/4939/changes |
ca83238 to
a414c9e
Compare
| async writeEntries() { | ||
| throw new Error('Use write instead for CompareResponseCache') | ||
| } |
There was a problem hiding this comment.
this one seems to be internal, can probably just be a method of the simple class and not of this one instead of being part of the interface
There was a problem hiding this comment.
The problem is that the compareCache takes the interface as input instead of the simple class
There was a problem hiding this comment.
seems more correct to me to have this expect an extended interface (or even the class) than to have this fail at runtime
| }) | ||
|
|
||
| test.serial( | ||
| 'composite transport merges child writes using shouldUpdate when run under an adapter', |
There was a problem hiding this comment.
would be nice to add a couple more test cases for the unhappy paths, e.g. having one transport fail to produce a value, requests coming with a specific transport request, etc
There was a problem hiding this comment.
Added 2 tests, not sure about "requests coming with a specific transport request", that doesn't seem to be related to the new transport added.
There was a problem hiding this comment.
Unless we took it out the framework supports specifying which transport to use when requesting (useful when the EA for example has a crypto endpoint and we know in advance a particular asset pair only exists in the rest transport and not the ws). I don't think this is a feature we can / should remove, and for safety I'd probably have the framework make sure that cases like these are supported, working, and not allowing for multiple instances of the transports that could cause problems (e.g. multiple ws connections, rate limiting failures to the provider)
| async writeEntries() { | ||
| throw new Error('Use write instead for CompareResponseCache') | ||
| } |
There was a problem hiding this comment.
seems more correct to me to have this expect an extended interface (or even the class) than to have this fail at runtime
| await Promise.all( | ||
| Object.entries(this.config.transports).map(([name, transport]) => | ||
| transport.initialize( | ||
| { ...dependencies, responseCache: compareCache }, | ||
| adapterSettings, | ||
| endpointName, | ||
| name, | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
One thing that came to mind when looking at the example usage PR in the ea monorepo (thanks for that btw, made this easier to reason even if very simple) is that we could mistakenly create multiple instances of the same adapter and initialize both, and potentially do unexpected things from the EAs like creating multiple connections to providers when we only intended/support one.
We need to add something on the framework side to check that if we're using a composite transport for a particular endpoint, either the underlying transports are the same as the composite one, we take the composite one and register the underlying ones automatically, etc.
| }) | ||
|
|
||
| test.serial( | ||
| 'composite transport merges child writes using shouldUpdate when run under an adapter', |
There was a problem hiding this comment.
Unless we took it out the framework supports specifying which transport to use when requesting (useful when the EA for example has a crypto endpoint and we know in advance a particular asset pair only exists in the rest transport and not the ws). I don't think this is a feature we can / should remove, and for safety I'd probably have the framework make sure that cases like these are supported, working, and not allowing for multiple instances of the transports that could cause problems (e.g. multiple ws connections, rate limiting failures to the provider)
Background: https://smartcontract-it.atlassian.net/browse/OPDATA-6402
Commit 1:
ResponseCacheintoResponseCache(base) andSimpleResponseCache(implementation)Commit 2:
Commit 3:
How to use this: https://github.com/smartcontractkit/external-adapters-js/pull/4939/changes