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 async channel API #55

Closed
wants to merge 25 commits into from
Closed

Core async channel API #55

wants to merge 25 commits into from

Conversation

danielcompton
Copy link
Collaborator

I deleted my fork of the clj-rethinkdb repo and didn't realise I still had async-chan on it. Reopening the PR from #37 here.

This PR adds support for a core.async channel API. It also rearranges a few things to make them tidier and easier to use from two different run functions. Of particular note, this upgrades protocol support to v4. This gives us the benefit of "long-polling" the TCP socket when we are :CONTINUEing, rather than having to keep sending queries every 500ms. More detail about this is at rethinkdb/rethinkdb#4400 (comment)

This PR isn't finished yet, in particular it needs

  • Documentation and a docstring, including a discussion of backpressure
  • Update the changelog
  • Tests
  • Testing from other people
  • A better understanding of the failure cases and how to recover from them. There is an :error-channel provided, but nothing will ever be put on it presently
  • A way to shut down queries and close the core.async channel the user is reading from when the connection is closed
  • Discussion about the API for this, in particular about how to provide channels and arity order. Should the user be able to provide an :error-channel and :control-channel too?
  • Probably moving run-query-chan to the rethinkdb.query namespace or adding a helper function
  • Discussion about a possible future async and current sync run API's. I think it might work well to build the other two on top of the core.async channel API, as there is subtle logic around :CONTINUE queries and we don't want to do this more than once. Alternatively I can imagine that there could be an async API which the other two layer on top of, but I'm not sure how it would actually work with :CONTINUE queries.
  • Discussion about using manifold instead of core.async for async work.
  • Testing of v4 protocol differences with v3
  • Fix broken tests
  • Send the type of changefeed along with the message
  • Create a buffered channel API which lets the user select how many results they want buffered ahead.
  • Add a lazy-seq API on top of the async-chan API
  • Add a callback based API on top of the async-chan API
  • Disconnecting the RethinkDB database causes a flurry of logging messages: 23:58:57.882 TRACE rethinkdb.net - Received raw response %s [0 ]. This is because it's trying to read on the socket. This is likely a pre-existing condition, but need to check it out further.

@danielcompton danielcompton changed the title Async chan Core async channel API Jul 2, 2015
@danielcompton danielcompton mentioned this pull request Jul 2, 2015
12 tasks
@danielcompton danielcompton self-assigned this Jul 2, 2015
@danielcompton
Copy link
Collaborator Author

544fe4a puts any error messages from RethinkDB on an error-chan.

@danielytics
Copy link
Contributor

Should we open an issue to discuss manifold vs core.async?

@danielcompton
Copy link
Collaborator Author

The only changes between v4 and v3 are its behaviour around sending multiple queries at the same time. Before the first one would need to return before any further queries would be returned, e.g. a stack. With v4 it returns whenever a query is done, and the driver needs to disambiguate (as we do already).

The other change is that on v3 a changefeed query would return after 500ms even if there were no changes to allow other queries to be executed (to avoid blocking the head of line). Now queries are held open permanently and don't return until there are changefeeds to report. This breaks the Cursor abstraction that we use.

Theres a few options here:

  1. Drop Cursors entirely, only support core.async
  2. Reimplement Cursors on top of core.async
  3. Fix the existing Cursor implementation
  4. Isn't really an option if we want to provide a sync API, as it's not possible to tell ahead of time which queries will be a :CONTINUE query. But I'm not sure how useful this is anyway, possibly we just throw an error if we get a sync query that returns a changefeed. Not a great option, but it may be ok and could encourage better patterns anyway.
  5. This is probably the "best" option as it preserves the Cursor abstraction, I'm not sure exactly what it needs, but it's an option
  6. Not really a fan of this option

@danielytics danielytics mentioned this pull request Jul 21, 2015
@danielcompton danielcompton force-pushed the async-chan branch 2 times, most recently from 016ef5a to 83037bf Compare August 6, 2015 06:42
@danielcompton danielcompton force-pushed the async-chan branch 4 times, most recently from d220bc3 to 5210efc Compare September 3, 2015 01:59
@KeeganMyers
Copy link

Is there any news on this PR? I have been working on an application with a non-blocking back-end that frequently uses core.async, and I would like to incorporate change feeds in a consistent manner. If you need a hand getting this into master I may be able to help depending on my workload.

@danielcompton
Copy link
Collaborator Author

@KeeganMyers I'm still working on this, GitHub's UI makes it look like I haven't done anything since June, but I'm chipping away at it if you look at commit times :) I'm currently using a SNAPSHOT for this, and you could too if you wanted to get this ahead of time (though I'd strongly recommend pinning to a specific SNAPSHOT version). As far as offers for help, that's really appreciated, and as I get closer to firming up the API that would be really helpful. Thanks!

* Rename connection channels
* Create add-to-waiting and remove-from-waiting functions for waiting
  queries
* Move some replace-vars to query-builder,
* Create prepare-query function to consolidate query-prep functions
Successful result, and error handling
Update tests to run against both query types
Make query response shapes more similar to standard driver
Relates to #97
Revert removing the (Thread/sleep 250) from cursors. I don't quite
understand it's purpose yet, but it is obviously needed.
- Rename changes arg from table to xs
- Add a new arity to changes to pass optargs

Fixes #112, Fixes #76
@danielcompton
Copy link
Collaborator Author

This PR is subsumed by the more recent manifold work. Closing as I'll do future async work on the mainline branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants