[streaming] ping related cleanup, inspection cleanup #3533

earce · 2020-05-23T22:01:41Z

Cleaning up some redundant parts of the code related to ping.

Log + Gemini both override method protected void handleIdle(ChannelHandlerContext ctx) which is now part of NettyStreamingService.java behavior so have removed these overrides. Since Gemini's idle time was 15 seconds which is Netty's default --have switched to use that default var.

In my QA environment I saw bitrich-info/xchange-stream#502 present itself and pingStatus treated as an unknown message type which I have now added to the switch to log as a warn given this presents itself when we are hitting rate limits. Not sure if another log level makes more sense? Additionally, is this issue only a product of attempting to open many WebSocket connections quickly, or can it occur by hammering a single WebSocket channel with 15+ subscriptions?

Have removed protected Completable openConnection() override from KrakenStreamingService.java given that the idle ping sends a ping out every 15 seconds of being idle (this will now be a web socket ping rather then an application level ping). @mdvx I saw you had added this in, with what frequency were you seeing disconnects? Was the heartbeat not sufficient to keep the connection alive?

In NettyStreamingService.java have cleared up some inspection warnings.

updating to head

updating fork

mdvx · 2020-05-24T01:58:29Z

@earce In my case, I can't tolerate the connection going down, and simply reconnecting, as I may miss updates, I need to clear my caches as soon as this is detected (and rebuild them after a fresh connect).

There are a couple PR from the old project I still need to bring over for this (I can only do one at a time)

earce · 2020-05-24T02:02:04Z

@mdvx I ran several tests with the iteration of the code in this review and never saw a disconnect.

Are the PRs you are referring to related to the issues with Kraken connectivity?

mdvx · 2020-05-24T02:36:27Z

No, not Kraken Specifically (but I can;t remember what prompted the changes). I have migrated them to this new PR now #3534

badgerwithagun · 2020-05-24T07:47:57Z

@mdvx are you comfortable with these changes, as they overlap with yours?

Are you aware that there is no way to either ensure the connection stays up or to detect if it drops 100% reliably? It's just not a feature of TCP.

I'm currently working on a background PR to add something I use in my app to the library: a wrapper for Exchange that adds backup polling when streaming and combines the two into a single stream. It's the best way I've found of resyncing streams when there are undetected lost frames. If you're interested I can put up a work-in-progress PR.

badgerwithagun · 2020-05-24T07:49:31Z

...ge-stream-kraken/src/main/java/info/bitrich/xchangestream/kraken/KrakenStreamingService.java

@@ -57,6 +54,9 @@ protected void handleMessage(JsonNode message) {
      KrakenEventType krakenEvent;
      if (event != null && (krakenEvent = KrakenEventType.getEvent(event.textValue())) != null) {
        switch (krakenEvent) {
+          case pingStatus:
+            LOG.warn("PingStatus received: {}", message);


Warn is quite high. Does this warrant waking someone up for?

changed to info

badgerwithagun · 2020-05-24T07:50:53Z

...vice-netty/src/main/java/info/bitrich/xchangestream/service/netty/NettyStreamingService.java

@@ -163,8 +163,7 @@ protected Completable openConnection() {
                if (ssl) {
                  SslContextBuilder sslContextBuilder = SslContextBuilder.forClient();
                  if (acceptAllCertificates) {
-                    sslContextBuilder =
-                        sslContextBuilder.trustManager(InsecureTrustManagerFactory.INSTANCE);
+                    sslContextBuilder.trustManager(InsecureTrustManagerFactory.INSTANCE);


Looks like the fmt plugin needs rerunning here

reran (nothing changed here but gemini streaming service did)

badgerwithagun

Just a couple of questions/spots

earce · 2020-05-24T14:54:31Z

@badgerwithagun yes typically with failed writes and timeouts, and this can be tougher to do depending on why the socket connection was broken.

Yes I'd be curious to see how you are combining polling and streaming. To your point of resyncing streams, are do you mean adding in polling to reconcile missed data or something else all together?

Yes I would be curious to see this PR.

@mdvx let me know if there is anything with this review that you need changed.

Also tangential but I know probably due to the ongoing crisis affecting every one the cadence of these releases has changed but does the project have an idea when 5.0.0 may be cut?

mdvx · 2020-05-24T23:15:00Z

@mdvx I saw you had added this in, with what frequency were you seeing disconnects? Was the heartbeat not sufficient to keep the connection alive?

It was disconnecting within the minute, every minute.

earce · 2020-05-24T23:18:43Z

@mdvx I saw you had added this in, with what frequency were you seeing disconnects? Was the heartbeat not sufficient to keep the connection alive?

It was disconnecting within the minute, every minute.

I didn't see any disconnects after running with this implementation for some time.

mdvx · 2020-05-24T23:23:04Z

We are all a bit out of sync, when I added the onIdle ctx write ping frame, the pingStatus message did not exist, it is possible we are doubling up now. I am installing the PR as it stands, with my other PRs and checking Gemini works

mdvx · 2020-05-24T23:34:10Z

I see no problems with this implementation :-) @earce @badgerwithagun

mdvx · 2020-05-24T23:49:51Z

Right now, I can't implement

class GeminiStreamingExchange { @Override public Observable<Throwable> reconnectFailure() { return streamingService.subscribeReconnectFailure(); } @Override public Observable<Object> connectionSuccess() { return streamingService.subscribeConnectionSuccess(); } }

because Gemini is using
private Map<CurrencyPair, GeminiProductStreamingService> productStreamingServices;

instead of a single GeminiProductStreamingService

any thoughts?

badgerwithagun · 2020-05-25T09:19:35Z

I'm merging this for now. Can we move your question to a separate issue, please, @mdvx?

I do think the info level logging for pingStatus is a bit excessive, but happy to leave that for now for Kraken users to debate over.

mdvx · 2020-05-25T14:50:12Z

#3537

earce and others added 4 commits May 21, 2020 18:34

Merge pull request #1 from knowm/develop

dd91b09

updating to head

cleanup of streaming classes, removing inspection errors

7f93a8e

Merge pull request #2 from knowm/develop

1c4f174

updating fork

Merge remote-tracking branch 'origin/develop' into develop

719b759

badgerwithagun reviewed May 24, 2020

View reviewed changes

badgerwithagun requested changes May 24, 2020

View reviewed changes

cleanup of streaming classes, removing inspection errors

8989b79

badgerwithagun approved these changes May 25, 2020

View reviewed changes

badgerwithagun merged commit f78f9cf into knowm:develop May 25, 2020

mdvx mentioned this pull request May 25, 2020

[Stream]Multiple NettyStreamingService error and (re)connection handling #3537

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[streaming] ping related cleanup, inspection cleanup #3533

[streaming] ping related cleanup, inspection cleanup #3533

earce commented May 23, 2020

mdvx commented May 24, 2020

earce commented May 24, 2020

mdvx commented May 24, 2020

badgerwithagun commented May 24, 2020

badgerwithagun May 24, 2020

earce May 24, 2020

badgerwithagun May 24, 2020

earce May 24, 2020

badgerwithagun left a comment

earce commented May 24, 2020 •

edited

Loading

mdvx commented May 24, 2020 •

edited

Loading

earce commented May 24, 2020 •

edited

Loading

mdvx commented May 24, 2020

mdvx commented May 24, 2020

mdvx commented May 24, 2020 •

edited

Loading

badgerwithagun commented May 25, 2020

mdvx commented May 25, 2020

[streaming] ping related cleanup, inspection cleanup #3533

[streaming] ping related cleanup, inspection cleanup #3533

Conversation

earce commented May 23, 2020

mdvx commented May 24, 2020

earce commented May 24, 2020

mdvx commented May 24, 2020

badgerwithagun commented May 24, 2020

badgerwithagun May 24, 2020

Choose a reason for hiding this comment

earce May 24, 2020

Choose a reason for hiding this comment

badgerwithagun May 24, 2020

Choose a reason for hiding this comment

earce May 24, 2020

Choose a reason for hiding this comment

badgerwithagun left a comment

Choose a reason for hiding this comment

earce commented May 24, 2020 • edited Loading

mdvx commented May 24, 2020 • edited Loading

earce commented May 24, 2020 • edited Loading

mdvx commented May 24, 2020

mdvx commented May 24, 2020

mdvx commented May 24, 2020 • edited Loading

badgerwithagun commented May 25, 2020

mdvx commented May 25, 2020

earce commented May 24, 2020 •

edited

Loading

mdvx commented May 24, 2020 •

edited

Loading

earce commented May 24, 2020 •

edited

Loading

mdvx commented May 24, 2020 •

edited

Loading