Redesign - #155
Conversation
| Unfetch(for { | ||
| a <- fa.run | ||
| b <- fb.run | ||
| result = (a, b) match { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Maybe we could implement the InMemoryCache with Ref
case class InMemoryCache(state: Ref[IO, Map[(String, Any), Any]) extends DataSourceCacheBut 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 { |
There was a problem hiding this comment.
(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
left a comment
raulraja
left a comment
There was a problem hiding this comment.
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.
| }) | ||
|
|
||
| override def fetch(id: String): IO[Option[Int]] = { | ||
| IO.delay(println(s"--> [${Thread.currentThread.getId}] One Length $id")) >> |
There was a problem hiding this comment.
Nothing wrong with IO.delay but the IO.apply already delegates to delay so you can express these just as:
IO(println(...))
There was a problem hiding this comment.
Indeed, I changed to IO#delay to be more explicit. I'll use ConcurrentEffect when I add #156 changes to this branch.
| 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]] |
There was a problem hiding this comment.
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
| implicit val cs: ContextShift[IO] = IO.contextShift(executionContext) | ||
|
|
||
| implicit override def executionContext = ExecutionContext.Implicits.global | ||
| implicit def ioToFuture[A](io: IO[A]): Future[A] = io.unsafeToFuture() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll use that instead, good point 👍
|
All checks passed |
|
This is great @purrgrammer, outstanding work. Merge at will! |
|
Do we plan on bumping the version to 2? |
|
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. |
Hide implementation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
2 similar comments
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
7 participants
This is still a WIP but I managed to rewrite the core
Fetchexecution model using an approach much closer to the original paper. The goal was to modernize the library and use the primitives fromcats-effect.I removed the
Querytype and I'm instead usingConcurrentEffectin data sources, it looks like:Paris a typeclass that eases usingcats.Parallel, taking only one type parameter instead of two. It comes from cats-par.I've gotten rid of
Freein 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 aFetchis encoded in theFetchResulttype which can beDoneorBlockedwaiting for a computation to finish before continuing. Having our own representation of the primitives we use fromFree(pure, flatMap) allows us to explore (and combine) both sides of a computation and group their requests.DataSourceCacheis also implemented in terms ofConcurrentEffectnow:which should solve #148 and allow users to use caches that perform
IOoperations. 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
Freeand a state-based interpreter, the new interpreter runs aFetchinstance into aConcurrentEffect.There are some features that I need to add before finishing, namely:
Monad[Fetch]#tailRecM1.0.0-RC1