Skip to content
This repository has been archived by the owner on Jun 8, 2020. It is now read-only.

Resubscribe channels only when reconnecting #548

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mladenmarkov
Copy link

Currently, the resubscribeChannels() is called on every successful connection.
This should be necessary only when reconnecting, i.e. in the scheduleReconnect() method.


connectionSuccessEmitters.forEach(emitter -> emitter.onNext(new Object()));
});
.doOnComplete(() -> connectionSuccessEmitters.forEach(emitter -> emitter.onNext(new Object())));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you check that this still works for exchanges such as Binance where channel subscriptions are made on connection?

() -> connect().subscribe(
() -> {
LOG.warn("Resubscribing channels");
resubscribeChannels();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to call the connectionSuccessEmitters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move over to PublishSubject for reconnFailEmitters and connectionSuccessEmitters? I know its a little out of scope, but would be nice. Maybe emit the exchange too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funnily enough, I just suggested that on another review! Yes, that seems sensible - it means that subscriptions to those observables could persist between manual reconnects.

@badgerwithagun badgerwithagun added the awaiting_fixes PR is awaiting submitter to respond to a review label Mar 27, 2020
@badgerwithagun badgerwithagun added the stale_final_warning This PR has been marked for closure unless progressed label May 3, 2020
@badgerwithagun
Copy link
Collaborator

This PR will be closed shortly unless there is further activity.

@badgerwithagun
Copy link
Collaborator

This project is in the process of being merged into the XChange project and no further PRs will be merged here. Once the projects have been merged, there may be a short stabilization period where there will be large-scale renaming of classes and packages, which may cause conflicts. You are advised to wait at least a week from now and then resubmit your PR on the XChange project. Thank you for your support!

@badgerwithagun
Copy link
Collaborator

You can now resubmit your PR on XChange. This project will shortly be marked as archived.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting_fixes PR is awaiting submitter to respond to a review stale_final_warning This PR has been marked for closure unless progressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants