stream/gateio: move adding and removing subscriptions from websocket wrapper to stream package #1717
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1717 +/- ##
==========================================
- Coverage 37.15% 37.13% -0.02%
==========================================
Files 414 414
Lines 180198 180199 +1
==========================================
- Hits 66950 66916 -34
- Misses 105389 105428 +39
+ Partials 7859 7855 -4
|
|
I have concerns about building this out further:
|
| if len(subs) != 0 { | ||
| if err = w.AddSuccessfulSubscriptions(conn, subs...); err != nil { | ||
| multiConnectFatalError = fmt.Errorf("%v Error adding successful subscriptions %w", w.exchangeName, err) | ||
| break | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think we can do this all-or-nothing approach.
I'm not sure we should bail on connect in either situation (multi or not), because failing to subscribe does not mean failing to connect. We should probably log the error and continue on.
But in this instance, if we managed to subscribe to 4 subs out of 5, we don't want to have not recorded that we subscribed to them.
As Subscriber is right now, I don't think we can delay Adding until here. It has to be in the manageSubs with more granular visibility.
This feels like it's raised a new issue for me of "connect shouldn't error if some subs fail", but that's separate.
There was a problem hiding this comment.
Yeah I differ in perspective on that, I regard a failure on 1 is a failure on the entire set. Especially when trading I want to ensure when I connect its subscribed to all the required subscriptions I want.
If there is a failure on subscription its either a malformed outbound payload or the response from the connection is delayed in both cases the connection is not ideal for trading ops. A retry under a for loop is easier and less complicated after handling error from connect if it is a subscription delay error.
There was a problem hiding this comment.
Feel free to return error from connect.
I don't agree with it, but sure.
Aside: The best solution to my problem with connect returning error is to not have connect do other things, and have Run call connect, then auth, then subscribe. Clearer responsibilities for methods.
I really can't get onboard with calling a method called connect, it connects, then does something else and returns an error about that other thing.
But for today, my problem is: Subs succeeded, but you haven't added any of them as successful.
If you want to return error from connect on sub failure, please ensure the successful ones are added first, then return error.
There was a problem hiding this comment.
Aside: The best solution to my problem with connect returning error is to not have connect do other things, and have Run call connect, then auth, then subscribe. Clearer responsibilities for methods.
I really can't get onboard with calling a method called connect, it connects, then does something else and returns an error about that other thing.
Well no the current design differs slightly from connect -> auth -> subscribe so we retrieve subscriptions -> only if subscriptions are available attempt connection/connect -> authenticate connection if available -> then subscribe (auth/unauth subs). We don't need to spawn connection/connect when there are no subscriptions, there is a caveat here for when this is allowed for a dedicated outbound connection for requests. This allows us to scale in the event of connection only allowing N subs per connection in future as well (OKX). Connect() means nothing without subscriptions. I guess we could rename to something different?
But for today, my problem is: Subs succeeded, but you haven't added any of them as successful.
If you want to return error from connect on sub failure, please ensure the successful ones are added first, then return error.
I don't know what you mean by this sorry? In regards to the current state of the PR I have added in AddSuccessfulSubscriptions but we are not going to do this any way? If this is directly related to I will investigate is check the connection subscription status after subscriptions to see if they have been added or removed. So that at least it complains that it needs to be added in the exchange packages themselves. If there are issues yeah I will address that for this.
I just want to steer this into a less complicated scenario for exchange implementations, pulling everything apart and toiling on things like this, seems like a good to have instead of a need to have. Everything you have said on your concerns has been on point. I am just trying to give a different perspective.
There was a problem hiding this comment.
Connect() means nothing without subscriptions.
We have non-subscription ws methods sometimes, right?
Or do you mean purely in the context of GateIO?
There was a problem hiding this comment.
I don't know what you mean by this sorry?
If this PR merges as it currently is, it means that subs which succeeded are not added as successful, if any failed.
Previously they were added as they succeeded by the exchange, one by one.
Now, if one of them fails, then L434 break means none of them is added, and that's wrong.
I don't know how you can do this outside of the exchange implementation, because it's so varied when and how we decide it's successful.
|
These are all fair points, an option I will investigate is check the connection subscription status after subscriptions to see if they have been added or removed. So that at least it complains that it needs to be added in the exchange packages themselves. |
shazbert
added
blocked
and removed
medium priority
review me
|
@shazbert Just realised that I haven't mentioned: I completely agree about abstracting from exchange implementations. I'm just not yet ready to see how to do it. I've been looking to abstract this, but I don't think we're ready yet because too much is in flight and unmerged. |
PR Description
Noticed in Bybit that subscriptions weren't being added when I was flushing the connection every hour, so abstracted adding and removing to websocket package then I can merge this to the open PR chain. This is so we don't need to toil in the wrappers as well but there might be some edge cases I am not considering.
This only impacts exchanges that are upgraded to the websocket multi connection.
Type of change
Please delete options that are not relevant and add an
xin[]as item is complete.How has this been tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.
Checklist