Support asynchronous data sources · Pull Request #54 · xebia-functional/fetch

ghost · 2016-05-29T17:36:18Z

Addresses #52. I also made an improvement: calling fetchOne and fetchMany depending on the number of the identities to be fetched. @raulraja please take a look!

Use Id as the target monad in the examples
Document all the available monad targets
Document Query constructors
Document how to bring your own concurrency monad
Simplify Query constructors: only leave sync (receiving a thunk), async and eval (lifts eval to synchronous query)

Edit: leaving more thorough testing as part of #32

~~- [ ] Test Monix integration more thoroughly~~

raulraja · 2016-05-29T18:52:57Z

I looked at the whole set of changes. Excellent work!
I have two major concerns with this PR.

We now depend in both places (Datasource, runX) on Task
We lost the ability to abstract over the target monad.

I think we need to:

Have our own type that wraps Task in this impl to wrap the async behavior we want to support in the DataSource
Have our own typeclass similar to MonadError but that supports the async behavior for easy conversions to Future, Task, etc... In cases where people would like to target Id or similar types where exceptions can't be captured they would be thrown. Cases where the async behavior forces a blocking behavior this would be user provided in the AsyncMonadError interface.

I think loosing the abstraction over the target monad it's not acceptable as that is one of the main features for me to use Fetch.

I think Task is a great type and we should use it internally but not leaking to the user types.
If the user does choose to lift to Task explicitly when the computation is evaled then it would be a toll free conversion. so the current behavior you are proposing won't be hurt or damaged in performance in any way.

What I'm trying to say is that there is no need to sacrifice abstracting over the return type while keeping the Task async and concurrent goodness. Thoughts?

ghost · 2016-06-01T21:28:00Z

Based on your suggestions i reworked the pull request and dropped the coupling with Task, making the target monad pluggable again. A summary of the current approach follows, let me know your thoughts.

The DataSource typeclass now looks as follows:

import cats.data.NonEmptyList

trait DataSource[Identity, Result]{
  def fetchOne(id: Identity): Query[Option[Result]]
  def fetchMany(ids: NonEmptyList[Identity]): Query[Map[Identity, Result]]
}

The Query type represents a query to a remote data source. It supports both synchronous data sources and asynchronous ones, the ADT is roughly as follows:

sealed trait Query[A] extends Product with Serializable

/** A query that can be satisfied synchronously. **/
final case class Sync[A](action: Eval[A]) extends Query[A]

type Callback[A] = A => Unit
type Errback     = Throwable => Unit

/** A query that can only be satisfied asynchronously. **/
final case class Async[A](action: (Callback[A], Errback) => Unit, timeout: Duration)
    extends Query[A]

As you can see, the synchronous queries can be lifted to the target concurrency monad with a plain .pureEval and, since we now support asynchronous data sources, a bit more work is needed to lift async queries to an arbitrary monad. Since the asynchronous computation is reduced to the bare minimum (a successful callback and an error callback) it should be trivial to integrate with any concurrency monad, even ones that hace synchronous semantics.

I still need to polish things here and there, i will write a task list in the PR description soon.

raulraja · 2016-06-01T21:38:53Z

This is awesome. I don't think we should use await in the docs. I think we should also provide examples for interpreting to Future, Task, Scalaz Task, etc... And synchronous ones with big warnings about their blocking and unsafe nature such as Id, Option, Eval, etc... In any case this is a great advancement in performance and design IMHO

ngbinh · 2016-06-02T15:35:35Z

this PR looks great! 👍

tomjadams · 2016-06-03T06:14:39Z

 ```

-Now that we have created a fetch, we can run it to a target monad. Note that the target monad (`Eval` in our example) needs to implement `MonadError[M, Throwable]`, we provide an instance for `Eval` in `fetch.implicits._`, that's why we imported it.
+Now that we have created a fetch, we can run it to a `Task`. Note that when we create a task we are not computing any value yet. Having a `Task` instance allows us to try to run it synchronously or asynchronously, choosing a scheduler.


I don't see any reference to Task in these code snippets. (Not being familiar with the codebase) I'm assuming that import fetch.implicits._ brings it in (or an implicit conversion from Fetch -> Task). It might be worth calling that out if so explicitly somewhere in the README.

tomjadams · 2016-06-03T06:22:46Z

Looks good from me, looking forward to playing with it.

ghost · 2016-06-05T13:22:27Z

Just finished documenting all of this, if you can proofread it and comment will be appreciated!

raulraja · 2016-06-05T13:42:26Z

 ```

-Now that we have created a fetch, we can run it to a target monad. Note that the target monad (`Eval` in our example) needs to implement `MonadError[M, Throwable]`, we provide an instance for `Eval` in `fetch.implicits._`, that's why we imported it.
+We'll run our fetches to the ambiend `Id` monad in our examples, let's do some imports.


raulraja · 2016-06-05T13:55:02Z

LGTM. Outstanding job!

tomjadams · 2016-06-06T10:38:29Z

+
 ```scala
-"com.fortysevendeg" %% "fetch" %% "0.2.0"
+"com.fortysevendeg" %% "fetch" % "0.2.0"


I'm guessing that these versions will change once a new build is made from this PR.

Yep, these changes will be released in a 0.3.0 version

tomjadams · 2016-06-06T19:58:11Z

I've left a whole bunch of comments on the README, mainly from a newbies "I don't understand" POV. Hope they're helpful :)

ghost · 2016-06-06T20:24:02Z

@tomjadams really valuable feedback, thanks a lot! I'll go through them tomorrow and make sure they get addressed.

tomjadams · 2016-06-06T23:39:07Z

I may have gone a bit nuts :) I just found a few things confusing from a beginners POV.
On Tue, Jun 7, 2016 at 6:24 AM, Aλ notifications@github.com wrote:
@tomjadams [https://github.com/tomjadams] really valuable feedback, thanks a lot! I'll go through them tomorrow and make sure they get addressed.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub [https://github.com//pull/54#issuecomment-224076661] , or mute the thread [https://github.com/notifications/unsubscribe/AABKZzjsw4g_YS235OGtMThZhFyo6V0gks5qJIHjgaJpZM4IpVmG] .

codecov-io · 2016-06-07T08:56:17Z

Current coverage is 60.84%

Merging #54 into master will decrease coverage by 16.51%

@@             master        #54   diff @@
==========================================
  Files             7          9     +2   
  Lines           106        189    +83   
  Methods         105        149    +44   
  Messages          0          0          
  Branches          1          5     +4   
==========================================
+ Hits             82        115    +33   
- Misses           24         74    +50   
  Partials          0          0

Powered by Codecov. Last updated by 608ad3f...d80da1f

ghost · 2016-06-13T10:55:43Z

I've clarified the Query API and its documentation. I'll leave testing monix more thoroughly to #32, since I plan to run the same property-based tests across all target types. @raulraja if you give me the thumbs up I'll merge this.

raulraja · 2016-06-13T10:57:59Z

LGTM!

raulraja · 2016-06-13T10:58:20Z

Again, amazing job!

ghost · 2016-06-13T11:02:02Z

ghost changed the title ~~Use monix.eval.Task instead of cats.Eval for supporting asynchronous data sources~~ Support asynchronous data sources May 31, 2016

tomjadams reviewed Jun 3, 2016
View reviewed changes

raulraja reviewed Jun 5, 2016
View reviewed changes

tomjadams reviewed Jun 6, 2016
View reviewed changes

Alejandro Gómez added 7 commits June 7, 2016 10:29

Depend on monix-eval

02cde66

Update DataSource to be implemented in terms of Task

8b43e76

Update docs

69081f8

Uniquify implicit name

e837ad8

Add tut compilation to CI

2dfa3b4

Minor changes in docs

6a14cc4

Update README

c22d13c

Alejandro Gómez added 14 commits June 7, 2016 10:33

Improve FetchMonadError[Future]

f5b83ab

Update docs to run fetches to Id

a58c251

Update README

3aef47b

Silence irrelevant output

be095c0

Document Query constructors

0c8ca32

Document Monix Task as the target monad

ba68bce

Improve FetchMonadError[Task] for Monix

1b3d118

Document how to bring your own concurrency monad

d2f99f2

Fix dependency specs

0d92468

Typo

158a12a

Concurrency monads

2234f68

🔥

a34ae16

🔥 are

ad842d6

Update README

d22eefe

ghost force-pushed the task-instead-of-eval branch from 4fbf2f7 to 8088021 Compare June 7, 2016 08:36

Alejandro Gómez added 2 commits June 7, 2016 10:40

Minor changes to README

8088021

Add monix tests to CI

b229d8f

Alejandro Gómez added 4 commits June 7, 2016 11:05

Unbreak Travis config

8525f67

Improvements to README

20172b0

Minor changes to DataSourceCache trait

00e0a7d

Simplify Query constructors

daa9d59

ghost force-pushed the task-instead-of-eval branch from 18b31fa to d80da1f Compare June 13, 2016 08:31

Report coverage after running JVM tests

d80da1f

ghost merged commit 100e5b9 into master Jun 13, 2016

ghost deleted the task-instead-of-eval branch June 13, 2016 11:02

This pull request was closed.

Uh oh!

Conversation

ghost commented May 29, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raulraja commented May 29, 2016

Uh oh!

ghost commented Jun 1, 2016

Uh oh!

raulraja commented Jun 1, 2016

Uh oh!

ngbinh commented Jun 2, 2016

Uh oh!

tomjadams Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

tomjadams commented Jun 3, 2016

Uh oh!

ghost commented Jun 5, 2016

Uh oh!

raulraja Jun 5, 2016

Choose a reason for hiding this comment

Uh oh!

raulraja commented Jun 5, 2016

Uh oh!

tomjadams Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

tomjadams commented Jun 6, 2016

Uh oh!

ghost commented Jun 6, 2016

Uh oh!

tomjadams commented Jun 6, 2016

Uh oh!

codecov-io commented Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 60.84%

Uh oh!

ghost commented Jun 13, 2016

Uh oh!

raulraja commented Jun 13, 2016

Uh oh!

raulraja commented Jun 13, 2016

Uh oh!

ghost commented Jun 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghost commented May 29, 2016 •

edited by ghost

Loading

codecov-io commented Jun 7, 2016 •

edited

Loading