Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Speed up rethinkdb driver issue #107 #108

Closed
wants to merge 7 commits into from

Conversation

KeeganMyers
Copy link

After benchmarking the driver I found that the inputStream based method for receiving responses from the server was causing a considerable slow down. Based on that issue I made some non-trivial modifications to rethinkdb.core & rethinkdb.net. Originally I had intended to use a light tcp abstraction clj-tcp. However, my naive attempts to parse messages failed when run in parallel. This was due mainly to the fact that tcp messages are not discreet and must be parsed on a frame by frame basis. So in the interest of time I brought gloss and aleph into the project.

Aleph makes use of manifold and gloss to buffer each frame and encode/decode it. Manifold streams can be coerced into core.async channels if/when it is required meaning that we can use each async mechanism when it is applicable.

I also made one slightly out of scope change to the queries to allow changes to accept parameters, this allows users to provide the squash parameter in a changefeed.

Let me know if you have any concerns, questions etc. This change set does introduce dependencies you may not be comfortable with so, I would be willing to keep this branch in my checkouts until interop with the official driver is complete.

@danielcompton
Copy link
Collaborator

Firstly, thanks heaps for this! I'm really happy someone has taken a look at the networking code as there have been some issues with it lately. I'm going to need to chew over the changes a little bit.

Heres my current thoughts:

  1. Adding new deps is not ideal, because this driver will be used by others who may have conflicting deps. Using Manifold or core.async #62 indicated people didn't really want extra deps.
  2. I've had issues in the past with these Ztellman libraries when updating to newer versions of Clojure. This would spread down to all consumers of this library too.
  3. The Java RethinkDB driver has just been released. Long term it would be great to offload all of the handling to them, I haven't looked yet at how difficult it will be to integrate it, or if they provide the right extensions points for other languages to piggy back on them.
  4. You've done the work, so I don't have to :)
  5. If we can't integrate with the Java driver easily, we could poach their connection handling code
  6. The current core.async based code doesn't seem to be working robustly (Memory leak #97, Busy hang #104) so moving somewhere else may be a good short term solution anyway.

I'll checkout the Java driver and have some hammock time on this.

in the meantime, could you squash all of the commits into one, and split off the changefeed commit into a separate PR which can fix #112?

Thanks heaps!

@danielcompton
Copy link
Collaborator

Hey, I don't think we need to close this just yet?

@danielcompton
Copy link
Collaborator

Will close this for now, as #114 seems to have provided a dramatic speedup, but may look into it again later.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants