Skip to content
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

[core][binance][livecoin] unified retry and rate limit proof of concept #3231

Merged
merged 33 commits into from
May 6, 2020

Conversation

walec51
Copy link
Collaborator

@walec51 walec51 commented Sep 18, 2019

work in progress - wait a moment with the review

pom.xml Outdated Show resolved Hide resolved
@walec51
Copy link
Collaborator Author

walec51 commented Sep 24, 2019

@badgerwithagun
Copy link
Collaborator

This looks fantastic at first look. Feel free to @ me for a review when done - I may want to pull support into xchange-stream.

@walec51
Copy link
Collaborator Author

walec51 commented Nov 13, 2019

@badgerwithagun I'm trying to push some of the features done here to resilience4j, currently you can help by stating your support here: resilience4j/resilience4j#643 ;)

@walec51 walec51 marked this pull request as ready for review April 27, 2020 12:38
@walec51
Copy link
Collaborator Author

walec51 commented Apr 27, 2020

@timmolter if you have some time please review this along with this documentation https://github.com/knowm/XChange/wiki/Implementing-resiliency-features

resiliency features are now configured in *Raw classes - previously I did it using annotation on proxy interfaces but my new annotation scheme was not accepted by the resilience4j project so I removed this way of doing things

@badgerwithagun
Copy link
Collaborator

@walec51 - one thing I've been wondering about recently is whether it would be possible to move XChange to a non-blocking core, allowing (optional) call semantics like:

exchange.getMarketDataService()
    .getTickerFuture(currency)
    .thenAccept(ticker -> {
      // Use the ticker
    });

The trick here being that this API would allow something like netty to be used, meaning no thread needs to block while the socket is waiting for a response from the exchange. After xchange-stream is merged, most of the wiring necessary to do this will already exist.

The reason I mention it here is to ask: does the solution you've been working on allow a promise-like approach to rate limiting which doesn't require a thread block? I understand resilience4j does have non-blocking support via the use of a small ScheduledExecutorService.

Obviously I'm not worried about supporting it now, it'd just be good to be able to see a migration route later.

@walec51
Copy link
Collaborator Author

walec51 commented Apr 28, 2020

@badgerwithagun resilience4j has some support for non blocking APIs but making XChange reactive would be a total rewrite of the library and all of its modules (additional APIs, totally different implementations, replacing mmazi/rescu with a less trivial proxy client library)

I don't see this happening any time soon and this should not be discussed in this PR

@walec51
Copy link
Collaborator Author

walec51 commented May 5, 2020

@timmolter is this PR ok with you? can I merge it?

@timmolter
Copy link
Member

yeah, looks good. Too bad they won't accept your PR. :(

@timmolter timmolter merged commit c5530ff into knowm:develop May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants