Problem with the connect()/disconnect() lifecycle in the Streaming API about xchange HOT 18 CLOSED

This is slightly improved in my implementation for mutiple channels:

Now, getEventQueue() must specify which API channel to connect to (ticker, trade, or market depth) when it is called. The user specifies an ExchangeEventType enum in the function call, which connect() uses to select the socket url. The eventQueue is then dedicated to that channel for the rest of the program run.

This at least implies to the user that an eventQueue should be assigned unique to each streaming socket.

Possible improvements:

1. Add disconnect() method in case user ever needs to terminate queue / connection.

2. Add "silent mode" to baseIO class or delete console output - the console output is rather gratuitous, especially when connecting more than one stream (e.g., trade stream and market depth stream).

3. Prevent multiple connections to same stream (e.g., multiple currencies), if necessary. I have not tested the new implementation with multiple connections (i.e., OP issue above) to the same channel (e.g., for multiple currencies), so I am not sure if baseIO would create a new stream/eventqueue or if it would just connect to the existing stream. If it creates a new stream, then there may not be any issue.

from xchange.

Hi Skipster555, I'll get back to you on this as soon as I have some free time, which will be in the next couple of days.

thanks much!!

from xchange.

This could introduce a difficult code review, I would suggest writing out the changes on a separate branch dedicated to this issue.

from xchange.

1. I think a disconnect() method makes sense
2. The mechanism to handle this is already in place with the logging. See logback.xml xchange-examples/src/main/resources:
   logger name="com.xeiam.xchange" level="DEBUG"
   set the level higher to INFO or WARN and the console noise will go away
3. It would be nice if we could send everything through one channel I think, but I don't know enough about the socketio API. This really needs someone to play around with it and see what works.

Gary, would you be willing to maybe take a look at the API again and either advise Skipster555, or make any quick API changes first? This streaming capability will be paramount for any trading bot that uses XChange.

Skipster555, I will shortly add you as a collaborator to XChange. This makes all of our lives easier. Please go ahead then and add a new branch for this issue as Gary suggested. Call it XChange-35 in reference to this issue. Then you won't have to do any pull requests either and I can just switch forks to look at code. It works well for Gary to if he has time to collaborate on this issue.

from xchange.

Skipster555.

I added you as a contributor and put you on the contributor list here: https://github.com/timmolter/XChange/wiki

You can now directly clone XChange's repo, brach and commit and push to any of the branches.

Cheers, Tim

from xchange.

Just a heads up to Tim and Skipster555 that I'm going to look at sorting this out tonight.

from xchange.

It looks as though Skipster555's work is based on the assumption that MtGoxStreamingMarketDataService is intended to also serve up trade and depth information. I think that might have come about due to the absence of the MtGoxStreamingTradeDataService and MtGoxStreamingDepthDataService which were intended to fulfil this role. This was to follow on from the implementation of trade and depth data but has not been communicated anywhere.

My bad.

from xchange.

Yes, I saw that too. I didn't want to mention it though and add more complication to Skipster555's efforts. But since you're in there, feel free to refactor it! :)

from xchange.

Or, should we package them all in one? Maybe that's better from a data handling perspective.

from xchange.

I'm currently in there adding examples, then on to figuring out testing.

1. Also, I think I may have broken everything by not updating the previous poms? Nothing seemed to build until I reversioned cored and mtgox to 1.4.1, even after re-cloning. Is that just me, or should I commit this?

2. In my implementation, I plan to create a struct DTO consisting of a ticker, trade, and a depth / order class, then create just two connections to trade and depth and extract what info I need to construct the three main market items. Not sure if a consolidated "Market Data Stream" DTO would be useful for you guys.

But after that, I think my personal implementation and the real world is calling, so not sure I'll have much more time to spend tracking this down.

from xchange.

1. yeah, that's weird. it should be all 1.4.1-SNAPSHOT. I noticed Skipster555's commits had 1.4.0 too, but my local pom always had 1.4.1-SNAPSHOT. Go ahead and commit and push whatever you think.
2. Go ahead and do whatever you think is best. I was just thinking out loud.

from xchange.

Oh, i'm an idiot. I'm a newb, so I had been working on a local copy of the code at first, which was 1.4.0.

Then I met git, and it decided to overwrite my files when first cloning (what must have been 1.4.1). I forked manually locally before forking on git.

I was lazy and just copied the core and mtgox folders from a previous local backup.

Could I have overwritten / missed any other changes from not being in sync?

from xchange.

OK, I've just caught up on these comments. I've added a new branch XChange-35 and pushed all my interim changes to it for review. Don't merge it! There is still work to be done for Trade and Depth services.

I'm in two minds about a consolidated market data service. On the one hand it does make things easier, but on the other we need to filter exchange events between 3 queues and not everyone will want the trade and depth info. I'm inclined to go with a dedicated service for each.

As a general note - the develop branch should have all code versioned as develop-SNAPSHOT. This never changes. When a release is pending a new release branch is made and then the version number is allocated in that branch. Have a look at the Branching Strategy wiki entry.

We've been a bit remiss in following it up until now, but I think we need to start getting strict with ourselves.

from xchange.

Just a note to myself. The getStreamingMarketDataService() calls need to have both a zero parameter and a single configuration option. This avoids a bunch of ugly getStreamingMarketDataService(null) requirements in the other exchanges that don't require configuration.

Once that's done the connect()/disconnect() refactoring should be complete (subject to code review). I can get around to this tonight (7 hours from now) or someone else can wade in and do it.

The Depth and Trade data can be implemented as separate issues, I think.

from xchange.

I've just pushed the final changeset for this issue. The MtGoxStreamingTickerDemo should now be able to serve up multiple ticker streams (BTC/USD and BTC/EUR in the demo) and uses the simplified connect()/disconnect() methods that should be common across all exchanges.

I've removed the ugly (null) entries and replaced them with default calls like was there before.

It's ready for code review and merging into develop.

from xchange.

Thanks a lot Gary. I'll look at it ASAP.

from xchange.

Thanks again for doing this, Gary. I had to comment out the innards of three mtgox streaming demo classes. They were giving compile errors, and I don't have time to investigate it right now.

from xchange.

I think the compile errors are coming from the StreamingMarketDepthDemo being renamed to MtGoxStreamingTickerDemo. You can safely delete the former.

from xchange.