-
Notifications
You must be signed in to change notification settings - Fork 186
WIP: Decorators - Load Balance #204
base: master
Are you sure you want to change the base?
WIP: Decorators - Load Balance #204
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together and sharing! This technique creates a nice mechanism for a more extensible BasicMultiClient
experience. I'm happy to explore this as a replacement for BasicMultiClient
till a more robust refactor architecture hammered out. This would necessitate implementing the required strategies for Bibox
, CEX
, and Coinex
to remove the BasicMultiClient
code.
The big test is always running subscribeTrades
or subscribeLevel2Updates
for all markets at one once. That functionality is implemented in the test suite but is only run against a handful of exchanges. During dev, I usually make a script instead of using the suite, as it's easier to debug.
In terms of structure, the only thing that I actively dislike is folder level index.js files.
In terms of using this as a general path forward the biggest issue as we talked about is bubbling up all complexity to the client level. In effect, this code is really just creating strategies for obtaining a socket for a subscription. I'm working on a facade prototype that I'll push up as a WIP PR as well.
Another broader issue is that event relay is going to increase overhead for each message. For high-volume clients (thousands per second in many cases) with a handful of strategies this could be a problem. Right now, the code already performs 3-4 layers of messages passing for a single message event websocket -> smartwss -> client ?-> multiclient -> consumer
. In the refactor I'm looking to flatten this and make smartwss functionality adjacent to the socket itself so that it's hopefully just websocket -> client -> consumer
.
* Also it have some utils and should had the repeated code across clients. | ||
*/ | ||
class AbstractClient extends EventEmitter { | ||
static relay(source, dest, channels = CHANNELS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I like this pattern for forward event emission.
@@ -842,4 +843,4 @@ function candlePeriod(period) { | |||
} | |||
} | |||
|
|||
module.exports = KucoinClient; | |||
module.exports = LoadBalanceClient.create((...args) => new KucoinClient(...args), { maxSubscriptions: 100 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice clean extensibility of the existing class! 🎉
Yes, the next iteration will implement Bibox, Cex and Coinex strategies.
I do same, use the own library to subscribe all trades. I tested with binance that's exchange with higher frequency events per seconds.
Why do u dislike it? Anyway, there's not a problem to change it.
This changes aimn't to solves client responsabilities problem, only abstract some little features that IMHO clients shouldn't know; like max subscriptions or balancing strategies. With barriers we can abstract and iterate this one. Great! I'm waiting for your WIP 👍
Yes there's a runtime overhead with multiple layers of
PS: Typescript refactor is must. 🤣 |
@fbadiola apologies for the delay, I just realized I had not yet replied! Re index.js, it's mostly a preference. The biggest reason being that outside of the module level it's an extra thing to maintain that is easy to forget about! In the past, it would also break intellisense in VSCode, not sure if it that's still the case though.
Awesome! Very nice work, thanks for testing it out concretely! Something to be mindful of. I've been playing around with mixins to assist with behavioral extension and have a similar feeling that deeply nested mixin hierarchies would have a similar negative impact on method invocation and property access (mostly based on my understanding of prototypical inheritance).
Absolutely! Especially when trying to write any type of composed code. Speaking of, I still haven't had a chance to put together a complete code example. A few other issue have cropped up to add to the complexity so I'm still hammering through all the variables. The generally idea is that one or more subscriptions gets paired with a live socket. The bundling exists for the duration of the socket. When a socket disconnects, the subscriptions for that socket goes back into a queue to be assigned to a new socket. This ensures reconnects behave the same as initial connections and don't inundate the exchange resulting in cascading failures. So the core client exists to control that process using various strategies. There are a few abstraction points I'm working to define:
For instance, for trade subscriptions for an exchange that requires us to send all markets in a single message, we may obtain a socket, debounce for a duration, then construct and send a single message for all markets. This behavior may be totally different for the same exchange when connecting to a private API endpoint to obtain your balance or execute a trade. Anyways, I digress... will hopefully have something up soon to share! Thanks again for the nice work you've put together and for spending time to help improve this library. Much appreciated! |
First implementation draft about decorators.
This draft aims to show a solution to enhance the functionalities of exchanges without modifying the current code and it could live while refactor the other parts.
There's changes on some core files like BasicClient and BasicMutiClient to returns boolean result on unsubscribe methods, like subscription methods do.
Pending:
Find good names to each part
Define folder structures
Move clients to AbstractClient*
Testing
AbstractClient pretends be like an abstract class on typescript. This is a first iteration to allow reuse more code between core clients and separate their responsabilities. Facade will be the next iteration of the refactor.
Suggestions and criticisms are welcome