[KRAKEN] Normalizing the approach for retry handling #1411
Conversation
|
Great stuff, generally:
|
|
Great stuff! Apologies for the text below which is about the context of the PR, not the contnt. The following is not a direct response to this PR, but about the direction I want Gekko to go in:
*After X retries (100 or a 1000 or so) it should stop retrying and upstream the error, not sure how to handle this when exchanges are temporary down. But not sure if trying to Most things in this PR are already in the same line as above. |
core/budfox/marketFetcher.js
Outdated
| @@ -72,7 +72,7 @@ Fetcher.prototype._fetch = function(since) { | |||
| if(++this.tries >= this.limit) | |||
| return; | |||
|
|
|||
| this.watcher.getTrades(since, this.processTrades, false); | |||
| util.retryForever((callback) => this.watcher.getTrades(since, callback, false), this.processTrades); | |||
There was a problem hiding this comment.
I'd rather keep this on the exchange level, if you are retrying inside kraken.js already, what is the need for this?
core/tools/dataStitcher.js
Outdated
| `Gekko tried to retrieve data since ${since.format('YYYY-MM-DD HH:mm:ss')}, however | ||
| ${provider} did not return any trades.` | ||
| ); | ||
| util.retryForever((callback) => { |
plugins/trader/portfolioManager.js
Outdated
| @@ -214,7 +214,8 @@ Manager.prototype.buy = function(amount, price) { | |||
| 'price:', | |||
| price | |||
| ); | |||
| this.exchange.buy(amount, price, this.noteOrder); | |||
|
|
|||
| util.retryForever((callback) => this.exchange.buy(amount, price, callback), this.noteOrder); | |||
plugins/trader/portfolioManager.js
Outdated
| @@ -245,7 +246,8 @@ Manager.prototype.sell = function(amount, price) { | |||
| 'price:', | |||
| price | |||
| ); | |||
| this.exchange.sell(amount, price, this.noteOrder); | |||
|
|
|||
| util.retryForever((callback) => this.exchange.sell(amount, price, callback), this.noteOrder); | |||
plugins/trader/portfolioManager.js
Outdated
| this.orders = []; | ||
| done(); | ||
| }); | ||
| util.retryForever((callback) => { |
plugins/trader/portfolioManager.js
Outdated
| @@ -363,7 +367,7 @@ Manager.prototype.relayOrder = function(done) { | |||
|
|
|||
| var getOrders = _.map( | |||
| this.orders, | |||
| order => next => this.exchange.getOrder(order, next) | |||
| order => next => util.retryForever((callback) => this.exchange.getOrder(order, callback), next) | |||
|
Maybe there is a difference in retrying at exchange level and at the portfolio level which I am not seeing, in that case please let me know why you did :) |
|
@askmike As you may have noticed over the weekend, there were some issue with the implementation of error and retry handling within kraken.js, when I think about the nature of the issue (ie: When should we retry, who decides we should retry) these are the criteria that come to mind:
The goal of this system was to meet those 3 criteria. The exchange tells us when to retry, the engine provides the implementation, reduces copy-paste code in the exchange function and reducing error. It opens the door the the engine deciding that a retry isn't appropriate (it: get ticker updating every frame could be implemented without the retry since it is already on a timer). That was my thinking :) |
|
I agree with the first 2 points, but I don't understand the third one:
That makes sense, but right now the upstream code just retries infinitely:
In case getTrades fails (which on Kraken has a big change of happening but definitely not on exchanges like bitfinex/poloniex), there is most likely something else wrong. See #869 and #1106 as examples to this. |
|
@askmike One of the cancel orders in portfolioManager doesn't have a retry at all, I'm pretty sure it would survive. But you are right, getTrades has other issues. It's a longer term goal that it could be addressed, if this was in place. The main argument IMHO is really about reducing bugs. I personally feel that exchange implementations will be more reliable long term with this approach since it simplifies the implementation, reduces copy and paste errors, etc. If you look at the current exchanges they don't all do retry in the same way, or even on the same functions. No one person is likely to refactor all exchange files if it can be avoided because there is risk in the amount of testing necessary to be reliable. This approach makes all improvements, present and future, available reliably to all exchange implementations. If you feel that this is not a priority, then it would probably make sense to move the util.retry calls into the exchange. |
|
I am 100% with you on decreasing bugs as the main priority! Though I think changing the behaviour of code that implements getTrades (by adding infinite retries) might possibly add new bugs on exchanges where getTrades not working means something being really wrong (IP being banned, as soon as they change their API, etc).
I agree that no one is going to go over all exchanges all at once and fix everything, but just infinitely retrying all exchanges (even the ones that are considered very stable like Poloniex, Bitstamp and Bitfinex, a few of the most used exchanges) is not a great solution to this imo.
This is the part where I think we should think about how we want
Right now the docs (=spec) does not define what the err should be, but we can change this right now into: the exchange implementation should retry unless the error is not recoverable (order was filled, order does not exist, etc). In which case the As soon as there is any issue with the |
|
@askmike Ok, I think I misunderstood your previous points. The issue is about the infinite retry, yes you are likely right. I did it this way to preserve what I saw in the exchange implementations, but fully agree that they should perhaps be something less intense. 3 retries seems a bit short, but perhaps 10 retries with a max of 30 seconds. This falloff I think is very safe from being banned. Additionally can use the helper still with the error types to give better control, and move these functions into the exchange itself, but that sort of implies doing the same thing for changes you made in the portfolioManager a few months back, we want all the retries on exchange functions in the exchange in that case right? This is definitely still an improvement I'll make the changes once I have your feedback. |
Ah! This is probably where the confusion came from: I shouldn't have add retries here, since it goes against the design (we both agree on). I'll try to correct this soon.
Yep!
A complete PR would be great, but I am already very happy about all of these changes (just not infi retry in getTrades consumers). But I'll gladly wait for more :) Your efforts are extremely appreciated! |
|
FYI, I am working on this, but it may take a day or two to complete and test what I had in mind. |
|
So this is what it looks like if we move the retry handling directly into the exchange. In this case we still use the helper function for handle retry, which gives us a nicer implementation of retry logic, but allows us to specify the retry configuration at exchange level since ideals are bound to vary from one implementation to another, and also preserves the upstreaming of irrecoverable and recoverable errors. If you are happy with these changes the next step will be for me to update the documentation to clarify the expectation of retry handling on the exchange implementation, and reference this implementation as a template for how to handle it.j I'd likely also take this myself into the Bitfnex and Binance exchanges since I use those regularly. But only after we've tested a bit in our stress test case (Kraken) :) As a bonus I also cleaned up a lot of the copy and paste code into a handler function, and standardized the formatting of the error messages. It look much nicer on the output when errors do occur. |
…cmroche-retry_handling
👍👍👍👍
I am very happy with the overall changes, however there were a few small issues I found while testing. I have fixed them here: https://github.com/cmroche/gekko/pull/1 If you agree & merge them they should show up here. |
|
Changes look great thanks @askmike, thanks. I will work on updating the documentation tonight and send a second PR for those changes probably over the weekend. |
small fixes in error handling for kraken (upstream #1411)
askmike
changed the title
|
@askmike gekko/core/util.js:187 ReferenceError: options is not defined |
|
See my solution in #1465, hope that works for you. |
Feature, will probably fix some bugs though
Currently retry handling is implemented by the individual exchanges, and so isn't very consistent. Depending on the implementation, some exchanges have irrecoverable errors to solve issues like API errors causing orders cancels to never end, others impement them in different functions. In Kraken, for instance, there was no retry handling on getTicker, but some other exchanges do implement this.
Also recent work (6 months ago) @askmike started to add retry handling directly in the portfolioManager. This is implemented fairly easily using the "retry" package which comes with feature not otherwise handled by our per exchange implementation, such as limiting the number of retries and exponential backoff of the retry delay.
In this PR I extended that functionality to all exchange functions, and was able to drop retry handling from kraken completely.
This approach is also fully compatible with exchange that currently have their own retry handling, so there is no need to update all the exchange implementations until someone is ready to test them.
This should help simplify the implementation and debugging of exchanges going forward.
Kraken's service is in such a state is disrepair right now that it was a convenient place to test the retry handlers worked correctly. Through various actions, importing, paper trading the recoverable errors are caught and dispatched through the retry handler as expected.