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

Redesign #155

Merged
merged 68 commits into from Sep 18, 2018

Conversation

Projects
None yet
7 participants
@purrgrammer
Collaborator

purrgrammer commented Aug 31, 2018

This is still a WIP but I managed to rewrite the core Fetch execution model using an approach much closer to the original paper. The goal was to modernize the library and use the primitives from cats-effect.

I removed the Query type and I'm instead using ConcurrentEffect in data sources, it looks like:

import cats.temp.par._
import cats.effect._

trait DataSource[I, A]{
  def name: String

  def fetch[F[_] : ConcurrentEffect : Par](id: I): F[Option[A]]

  /* automatically implemented in terms of `fetch` and parTraverse, user can override */
  def batch[F[_] : ConcurrentEffect : Par](ids: NonEmptyList[I]): F[Map[I, A]] 
}

Par is a typeclass that eases using cats.Parallel, taking only one type parameter instead of two. It comes from cats-par.

I've gotten rid of Free in our code, since it didn't allow us to explore both sides of a computation in the way this encoding does. Now a description of a Fetch is encoded in the FetchResult type which can be Done or Blocked waiting for a computation to finish before continuing. Having our own representation of the primitives we use from Free (pure, flatMap) allows us to explore (and combine) both sides of a computation and group their requests.

DataSourceCache is also implemented in terms of ConcurrentEffect now:

trait DataSourceCache {
  def lookup[F[_] : ConcurrentEffect, I, A](i: I, ds: DataSource[I, A]): F[Option[A]]
  def insert[F[_] : ConcurrentEffect, I, A](i: I, v: A, ds: DataSource[I, A]): F[DataSourceCache]
}

which should solve #148 and allow users to use caches that perform IO operations. I still need to write a proof-of-concept Redis cache integration to make sure the API makes sense.

Since I've moved away from Free and a state-based interpreter, the new interpreter runs a Fetch instance into a ConcurrentEffect.

There are some features that I need to add before finishing, namely:

  • Monad[Fetch]#tailRecM
  • Missing identities
  • Add caching
  • Test error handling
  • Test batching
  • Syntax
  • Update environment debug code
  • Update documentation
  • Update examples (blocked until http4s supports cats-effect >= 1.0.0)
  • Update README
  • Scala.js support
  • Coverage
  • Bump version to 1.0.0-RC1
Unfetch(for {
a <- fa.run
b <- fb.run
result = (a, b) match {

This comment has been minimized.

@fedefernandez

fedefernandez Sep 3, 2018

Contributor

Can this be expressed with an apply product? If it's possible I think it makes more sense.

(fa.run, fb.run).tupled.map {
  case (Done(a), Done(b)) => Done((a, b))
  //...
case class InMemoryCache(state: Map[DataSourceIdentity, Any]) extends DataSourceCache {
override def get[A](k: DataSourceIdentity): Option[A] =
state.get(k).asInstanceOf[Option[A]]
case class InMemoryCache(state: Map[(String, Any), Any]) extends DataSourceCache {

This comment has been minimized.

@fedefernandez

fedefernandez Sep 3, 2018

Contributor

Maybe we could implement the InMemoryCache with Ref

case class InMemoryCache(state: Ref[IO, Map[(String, Any), Any]) extends DataSourceCache

But in this case, you would need to find the place to instantiate this cache as a side effect (Ref.of[IO, Map[(String, Any), Any](Map.empty) returns IO

case class InMemoryCache(state: Map[DataSourceIdentity, Any]) extends DataSourceCache {
override def get[A](k: DataSourceIdentity): Option[A] =
state.get(k).asInstanceOf[Option[A]]
case class InMemoryCache(state: Map[(String, Any), Any]) extends DataSourceCache {

This comment has been minimized.

@fedefernandez

fedefernandez Sep 3, 2018

Contributor

(String, Any) represents the DataSource name and the "key", right?

Can we define both with a value classes?

final class DataSourceName(val name: String) extends AnyVal
final class DataSourceKey(val key: Any) extends AnyVal
@raulraja

Excellent work and great step fwd toward a better runtime for Fetch. Kudos! 👏 👏 . Once we have a final impl merged in master we will redo https://github.com/47deg/kollect in the same way now that Arrow has an Effect library.

README.md Outdated
})
override def fetch(id: String): IO[Option[Int]] = {
IO.delay(println(s"--> [${Thread.currentThread.getId}] One Length $id")) >>

This comment has been minimized.

@raulraja

raulraja Sep 11, 2018

Member

Nothing wrong with IO.delay but the IO.apply already delegates to delay so you can express these just as:

IO(println(...))

This comment has been minimized.

@purrgrammer

purrgrammer Sep 11, 2018

Collaborator

Indeed, I changed to IO#delay to be more explicit. I'll use ConcurrentEffect when I add #156 changes to this branch.

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

This comment has been minimized.

@raulraja

raulraja Sep 11, 2018

Member

Even if Fetch internal implementation is on IO I believe there is no need to force users to use concrete data types if we define data similar to:

abstract class DataSource[F[_]: Effect, G[_], Identity, Result](implicit P: Parallel[F, G]) {
  def name: String
  def fetch(id: Identity): F[Option[Result]]
  def batch(ids: NEL[Identity]): F[Option[Result]]
}

This would allow users to implement their data sources and consume Fetch with other datatypes in that provide instances for Effect beside IO because Effect#runAsync returns an IO value that you can use internally and back to the context of the user to F via LiftIO

This comment has been minimized.

@purrgrammer

purrgrammer Sep 11, 2018

Collaborator

Done in #156

implicit override def executionContext = ExecutionContext.Implicits.global
implicit def ioToFuture[A](io: IO[A]): Future[A] = io.unsafeToFuture()

This comment has been minimized.

@raulraja

raulraja Sep 11, 2018

Member

We have been bitten before by this kind of implicit conversion and usually prefer explicit calls to unsafetoFuture in tests that expect a future back and it's explicit when things run in tests.

This comment has been minimized.

@purrgrammer

purrgrammer Sep 11, 2018

Collaborator

I'll use that instead, good point 👍

purrgrammer added some commits Sep 11, 2018

@purrgrammer

This comment has been minimized.

Collaborator

purrgrammer commented Sep 14, 2018

All checks passed :shipit:

@raulraja

This comment has been minimized.

Member

raulraja commented Sep 14, 2018

This is great @purrgrammer, outstanding work. Merge at will!

@peterneyens

This comment has been minimized.

Member

peterneyens commented Sep 17, 2018

Do we plan on bumping the version to 2?

@purrgrammer

This comment has been minimized.

Collaborator

purrgrammer commented Sep 17, 2018

I was planning to bump the version to 1.0.0 when we merge these changes, they are pretty substantial and deserve a major version change.

This was referenced Sep 18, 2018

@codecov

This comment has been minimized.

codecov bot commented Sep 18, 2018

Codecov Report

Merging #155 into master will increase coverage by 12.41%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #155       +/-   ##
==========================================
+ Coverage   78.09%   90.5%   +12.41%     
==========================================
  Files          16       5       -11     
  Lines         315     158      -157     
  Branches        2       5        +3     
==========================================
- Hits          246     143      -103     
+ Misses         69      15       -54
Impacted Files Coverage Δ
shared/src/main/scala/syntax.scala 100% <100%> (+33.33%) ⬆️
shared/src/main/scala/datasource.scala 100% <100%> (+33.33%) ⬆️
shared/src/main/scala/env.scala 66.66% <66.66%> (+16.66%) ⬆️
shared/src/main/scala/cache.scala 80% <85.71%> (-4.62%) ⬇️
shared/src/main/scala/fetch.scala 91.36% <91.36%> (+3.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b85a21...3fe15b3. Read the comment docs.

2 similar comments
@codecov

This comment has been minimized.

codecov bot commented Sep 18, 2018

Codecov Report

Merging #155 into master will increase coverage by 12.41%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #155       +/-   ##
==========================================
+ Coverage   78.09%   90.5%   +12.41%     
==========================================
  Files          16       5       -11     
  Lines         315     158      -157     
  Branches        2       5        +3     
==========================================
- Hits          246     143      -103     
+ Misses         69      15       -54
Impacted Files Coverage Δ
shared/src/main/scala/syntax.scala 100% <100%> (+33.33%) ⬆️
shared/src/main/scala/datasource.scala 100% <100%> (+33.33%) ⬆️
shared/src/main/scala/env.scala 66.66% <66.66%> (+16.66%) ⬆️
shared/src/main/scala/cache.scala 80% <85.71%> (-4.62%) ⬇️
shared/src/main/scala/fetch.scala 91.36% <91.36%> (+3.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b85a21...3fe15b3. Read the comment docs.

@codecov

This comment has been minimized.

codecov bot commented Sep 18, 2018

Codecov Report

Merging #155 into master will increase coverage by 12.41%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #155       +/-   ##
==========================================
+ Coverage   78.09%   90.5%   +12.41%     
==========================================
  Files          16       5       -11     
  Lines         315     158      -157     
  Branches        2       5        +3     
==========================================
- Hits          246     143      -103     
+ Misses         69      15       -54
Impacted Files Coverage Δ
shared/src/main/scala/syntax.scala 100% <100%> (+33.33%) ⬆️
shared/src/main/scala/datasource.scala 100% <100%> (+33.33%) ⬆️
shared/src/main/scala/env.scala 66.66% <66.66%> (+16.66%) ⬆️
shared/src/main/scala/cache.scala 80% <85.71%> (-4.62%) ⬇️
shared/src/main/scala/fetch.scala 91.36% <91.36%> (+3.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b85a21...3fe15b3. Read the comment docs.

@purrgrammer purrgrammer merged commit 6ecea36 into master Sep 18, 2018

4 checks passed

codecov/patch 90.9% of diff hit (target 78.09%)
Details
codecov/project 90.5% (+12.41%) compared to 9a93aac
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@purrgrammer

This comment has been minimized.

Collaborator

purrgrammer commented Sep 18, 2018

:shipit:

@codecov

This comment has been minimized.

codecov bot commented Sep 18, 2018

Codecov Report

Merging #155 into master will increase coverage by 12.41%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #155       +/-   ##
==========================================
+ Coverage   78.09%   90.5%   +12.41%     
==========================================
  Files          16       5       -11     
  Lines         315     158      -157     
  Branches        2       5        +3     
==========================================
- Hits          246     143      -103     
+ Misses         69      15       -54
Impacted Files Coverage Δ
shared/src/main/scala/syntax.scala 100% <100%> (+33.33%) ⬆️
shared/src/main/scala/datasource.scala 100% <100%> (+33.33%) ⬆️
shared/src/main/scala/env.scala 66.66% <66.66%> (+16.66%) ⬆️
shared/src/main/scala/cache.scala 80% <85.71%> (-4.62%) ⬇️
shared/src/main/scala/fetch.scala 91.36% <91.36%> (+3.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b85a21...3fe15b3. Read the comment docs.

@purrgrammer purrgrammer deleted the redesign branch Sep 18, 2018

@codecov

This comment has been minimized.

codecov bot commented Sep 18, 2018

Codecov Report

Merging #155 into master will increase coverage by 12.41%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #155       +/-   ##
==========================================
+ Coverage   78.09%   90.5%   +12.41%     
==========================================
  Files          16       5       -11     
  Lines         315     158      -157     
  Branches        2       5        +3     
==========================================
- Hits          246     143      -103     
+ Misses         69      15       -54
Impacted Files Coverage Δ
shared/src/main/scala/syntax.scala 100% <100%> (+33.33%) ⬆️
shared/src/main/scala/datasource.scala 100% <100%> (+33.33%) ⬆️
shared/src/main/scala/env.scala 66.66% <66.66%> (+16.66%) ⬆️
shared/src/main/scala/cache.scala 80% <85.71%> (-4.62%) ⬇️
shared/src/main/scala/fetch.scala 91.36% <91.36%> (+3.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b85a21...3fe15b3. Read the comment docs.

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