Add cats-effect to the core module, remove modes #345

kubukoz · 2020-02-07T01:14:18Z

No description provided.

modules/core/shared/src/main/scala/scalacache/AbstractCache.scala

lewisjkl · 2020-02-07T02:39:05Z

@kubukoz this is looking great! Feels so much cleaner without the modes 😅.

cb372 · 2020-02-07T09:39:48Z

Great! The number of deleted lines is a good sign :) I’m on my phone now but I can take a proper look this weekend.

kubukoz · 2020-02-07T13:12:10Z

@cb372 please do, if it looks good I'll proceed to migrate remaining modules.

cb372

Looks great so far. Please continue 😄

cb372 · 2020-02-08T22:57:04Z

modules/caffeine/src/main/scala/scalacache/caffeine/CaffeineCache.scala

-        else
-          Some(entry.value)
+  def doGet(key: String): F[Option[V]] = {
+    F.delay {


nit: let's write one-liners like this as F.delay(...) instead of with curly braces.

I have a personal preference of putting by-name parameters in curly braces (if the parameter list only has one parameter), especially when lifting side effects. But I can adjust :)

modules/core/shared/src/main/scala/scalacache/AbstractCache.scala

cb372 · 2020-02-08T23:08:30Z

modules/core/shared/src/main/scala/scalacache/AbstractCache.scala

@@ -49,123 +55,92 @@ trait AbstractCache[V] extends Cache[V] with LoggingSupport {
      if (logger.isDebugEnabled) {
        logger.debug(s"Skipping cache PUT because cache writes are disabled. Key: $key")
      }
-      mode.M.pure(())
+      F.pure(())


That'll need .widen - F is invariant and we need F[Any] unless I change the interface's type. Which I want to do, but maybe as a different change.

cb372 · 2020-02-08T23:09:41Z

modules/core/shared/src/main/scala/scalacache/CacheAlg.scala

    */
-  def close[F[_]]()(implicit mode: Mode[F]): F[Any]
+  //TODO: Replace with Resource-based API?


Possibly, but let's save that for another PR

lewisjkl · 2020-02-13T04:31:45Z

if it looks good I'll proceed to migrate remaining modules.

I know we talked in #336 about potentially removing some of the cache implementations. Could be worth drawing a box around which ones we'd want to remove (if any) prior to doing the work of actually migrating them. If so, I can open another issue so we can come to a decision about which ones to potentially remove.

kubukoz · 2020-02-13T20:59:05Z

@lewisjkl I'm totally on board with that, no point migrating code that is going to disappear. If you have some thoughts on which ones we'll remove, please bootstrap an issue :)

lewisjkl · 2020-02-17T03:07:50Z

@kubukoz I just created #353 to discuss which cache implementations should be removed.

_{Sent with GitHawk}

cb372 · 2020-02-25T14:36:26Z

Hey @kubukoz thanks for waiting. We've now removed a few modules, so feel free to rebase and pick this work up again when you have time.

kubukoz · 2020-03-15T20:25:13Z

Hey, I'll try to get back to this soon. Finished some other work so my bandwidth is a bit wider now...

kubukoz · 2020-03-22T18:56:14Z

@cb372 @lewisjkl how do you feel about logging? Currently we have impure logging all over the place, and it should be suspended.

I see the following options as best:

keep the current logging but lift it manually via Sync
depend on log4cats (it'll increase the amount of work to start using scalacache, as users will need to create a logger themselves)

For convenience, I'd go with the first, but not everyone will want to use slf4j (as we do now), which might be a problem later. What do you think?

lewisjkl · 2020-03-23T02:32:19Z

depend on log4cats (it'll increase the amount of work to start using scalacache, as users will need to create a logger themselves)

This would be great if not for requiring that extra setup. Do you think there’d be any good way to set a default to a no-op logger that is overridable if a user desires? If so, is that something worth doing?

Otherwise the sync option could be safer for now until we can think of a way to lower the friction to integrate and provide logging decoupled from a logging backend. I can definitely be persuaded here though.

kubukoz · 2020-03-23T03:54:10Z

I think it's a reasonable course of action to proceed without anything smarter than Sync for now. Before the big bang release we can think about having it configurable. A no-op default certainly sounds possible in a way that makes it easy to override, as people don't create new caches all around the place (so less risk of inconsistent instances across the codebase).

lewisjkl · 2020-05-27T20:07:12Z

@kubukoz is there anything that I can do to help push this along? I'm not trying to push you to do anything here, just know that I am happy to help if you don't currently have the bandwidth 😄

kubukoz · 2020-05-27T22:55:06Z

@kubukoz is there anything that I can do to help push this along? I'm not trying to push you to do anything here, just know that I am happy to help if you don't currently have the bandwidth 😄

I'd love to finally do this, as it's been open for a while... I'll get back to it so I'm not blocking further improvements any longer.

cb372 · 2020-05-28T09:21:08Z

Yeah it would be great to get this one over the line 😄 Let me know if I can help.

By traversing on the entry's TTL we get the opportunity to avoid calculating the time in case there's no TTL. That might give us some extra performance in case of infinitely long-lived keys. The constraint is more powerful, but all the caching implementations will be at least monads, so it's fine. And Clock is only possible to implement in a real scenario for instances of Sync.

kubukoz · 2020-05-31T17:58:42Z

Apparently the redis cluster tests aren't working yet, and I haven't been able to set it up locally yet... I'll see what I can do.

lewisjkl · 2020-06-01T15:31:20Z

modules/core/shared/src/main/scala/scalacache/Entry.scala

+      .map {
+        case None | Some(true) => true
+        case Some(false)       => false
+      }


What about .forall(identity)?
Maybe that would just be harder to read, but thought I'd throw it out there.

Yeah, I had that but decided an explicit match would be more readable (in fact, I had a bug here due to having used contains / exists).

Cool, yeah I agree that the match is more readable. You don't have to do the mental gymnastics of remembering forall defaults to true.

lewisjkl · 2020-06-01T15:44:30Z

@kubukoz thanks for going through all all of that! Just looked through and didn't see any missed delays or *>. I'd be happy to take a look at the redis tests as well, but I don't want to step on your toes either. Just lmk if I can help 🙂

kubukoz · 2020-06-01T15:47:06Z

@kubukoz thanks for going through all all of that! Just looked through and didn't see any missed delays or *>. I'd be happy to take a look at the redis tests as well, but I don't want to step on your toes either. Just lmk if I can help 🙂

I'll give it one more attempt today and let you know how it went :)

paulpdaniels · 2020-06-19T05:45:32Z

modules/core/jvm/src/main/scala/scalacache/logging/Logger.scala

@@ -1,23 +1,26 @@
 package scalacache.logging

 import org.slf4j.{Logger => Slf4jLogger, LoggerFactory}
+import cats.effect.Sync
+import cats.Applicative
+import cats.implicits._


You don't appear to need this, based on what you are using I'd suggested targeted syntax:

import cats.syntax.option._ import cats.syntax.flatMap._

paulpdaniels · 2020-06-19T05:48:25Z

modules/core/shared/src/main/scala/scalacache/AbstractCache.scala

@@ -3,6 +3,11 @@ package scalacache
 import scala.concurrent.duration.Duration

 import scala.language.higherKinds
+import cats.Monad
+import cats.implicits._


targeted syntax for functor, monadError and flatMap

I'm not sure if there is a global standard for whether we want to use fine-grained imports or the whole thing. I can switch to either, but is there any improvement in using the targeted ones? The compilation time is roughly the same, from what I saw last time I checked.

I try to use targeted imports as much as possible. Partially for compilation speed, although that is only anecdotal and wouldn't matter anyway on a project of this size. But more importantly for readability, as it gives readers more of a clue where instances and extension methods are coming from.

paulpdaniels · 2020-06-19T05:55:09Z

modules/core/shared/src/main/scala/scalacache/Cache.scala

-      f: => F[V]
-  )(implicit mode: Mode[F], flags: Flags): F[V]
+  def cachingForMemoizeF(baseKey: String)(ttl: Option[Duration])(
+      f: F[V]


Call by name might still be useful here, since I don't think there is a restriction against using Future or Try is there?

Neither Future nor Try have cats-effect instances, which you need anyway (Sync, for some impls. Async).

cb372 · 2020-06-27T15:10:52Z

Sorry everyone, I should have been pushing this PR a bit harder but I keep getting distracted.

@lewisjkl do you want to pick it up and try to get the tests working so we can get it merged and start working towards a release?

@kubukoz Thank you for all your work so far!

lewisjkl · 2020-06-27T15:30:43Z

@lewisjkl do you want to pick it up and try to get the tests working so we can get it merged and start working towards a release?

I would be happy to pick this up. I’ll take a look within the next couple of days and see if I can get those tests passing.

lewisjkl · 2020-06-29T20:01:54Z

Started to look at the tests this morning. So far I tried to get a local redis cluster set up using docker-compose. I am able to get the tests in the integrationtests package to pass just fine, but the ones in RedisClusterCacheSpec are just hanging when I run them. I noticed that there is a make file in the project for setting up with Travis, but I am on a Mac rather than Linux so it didn't seem completely well suited for me. How do you typically go about running these locally @cb372? I am thinking I will keep going with the docker stuff I have up unless you have a better or more proven way.

cb372 · 2020-07-02T09:01:44Z

@lewisjkl I don't think I've ever run the cluster tests locally. Somebody else contributed that makefile and I've always relied on Travis to run those test for me.

In the past I installed redis via brew and fiddled around with config files to get Redis Sentinel working, but never tried for cluster. Docker sounds like a better idea.

lewisjkl · 2020-07-02T13:10:01Z

@cb372 sounds good, thanks for the info. Mostly just wanted to make sure I wasn't reinventing the wheel here. I will see what I can get figured out with Docker. If I come up with something noteworthy then I will add it in a PR sometime soon.

lewisjkl · 2020-07-03T14:25:13Z

So now all of the tests are passing and the docs are compiling. With the docs, I do think that we should go through and reformat some of the sections. Just to get them compiling, I added things like .unsafeRunSync when it is probably not preferable, but it will be a slightly larger rewrite to get them working using .flatMap/regions of sharing. Do you think I should take the time to do that on this PR? Or should we split that out so we can get this one merged?

I also noticed that there was a todo or two in the changeset for this PR. I will track them down and open individual comments about them in this thread so we can ensure they are addressed.

lewisjkl · 2020-07-03T14:27:31Z

modules/core/shared/src/main/scala/scalacache/Cache.scala

@@ -3,19 +3,20 @@ package scalacache
 import scala.concurrent.duration.Duration
 import scala.language.higherKinds

-trait Cache[V] extends CacheAlg[V] {
+//todo merge with alg?


Is this something we want to address in this PR? Seems like a good candidate for another PR to me, but happy to hear other opinions.

Yes let's do this in another PR if we decide it's a good idea. I don't really remember why I separated Cache and CacheAlg in the first place.

lewisjkl · 2020-07-03T14:28:55Z

modules/core/shared/src/main/scala/scalacache/CacheAlg.scala

@@ -7,74 +7,64 @@ import scala.language.higherKinds
 /**
  * Abstract algebra describing the operations a cache can perform
  *
+  * @tparam F the effect of the cache. //todo elaborate


I don't know how much elaboration is needed here. I think going too much more into depth on what F[_] means could be out of scope here.

Yeah I think we can just remove the @tparam line completely. We'll assume that anyone using the library is familiar with tagless-final.

lewisjkl · 2020-07-03T14:31:37Z

modules/core/shared/src/main/scala/scalacache/package.scala

@@ -3,6 +3,7 @@ import scala.language.higherKinds

 package object scalacache {

+  //todo


Is this todo indicating that type Id[X] = X is able to be removed? I am guessing that it can be removed in favor of people using cats Id when appropriate. Although it won't be usable for any of the algebras requiring Sync/Async anyway..

Yes I think this can be removed. I'll add a commit to do that.

lewisjkl · 2020-07-03T14:33:46Z

Also, for anyone curious, I ended up just using this repo (I also removed the password that is set by default in this repo) to test with Redis locally.

cb372 · 2020-07-31T15:37:12Z

It finally happened! Merged! 🚀

kubukoz · 2020-07-31T16:23:27Z

Wow, thanks a lot for making it happen @lewisjkl @cb372! I'm sorry I didn't get this to the end...

lewisjkl · 2020-07-31T16:26:49Z

Wow, thanks a lot for making it happen @lewisjkl @cb372! I'm sorry I didn't get this to the end...

No worries! Thanks for helping out so much with the refactor.

kubukoz mentioned this pull request Feb 7, 2020

Let's save scalacache! #336

Open

kubukoz commented Feb 7, 2020

View reviewed changes

modules/core/shared/src/main/scala/scalacache/AbstractCache.scala Outdated Show resolved Hide resolved

cb372 reviewed Feb 8, 2020

View reviewed changes

This was referenced Feb 11, 2020

zio integration #307

Closed

Adding helper function to instanciate caches within Cats Resource #220

Closed

lewisjkl linked an issue Feb 17, 2020 that may be closed by this pull request

Remove Cache Implementations #353

Closed

lewisjkl mentioned this pull request Feb 22, 2020

Update scalafmt-core to 2.4.1 #351

Closed

kubukoz force-pushed the cats-effect branch from 4c52ee5 to 6892d82 Compare March 22, 2020 18:00

lewisjkl mentioned this pull request Apr 20, 2020

Is there a getAll method? #379

Open

kubukoz added 7 commits May 31, 2020 17:54

Add cats-effect to the core module, remove modes

ac21331

Port caffeine module

30dafe5

Remove ohc again

b88d5cb

Restore line

3d5438b

Update cats-effect to 2.1.2

19495b4

Add Sync requirement to abstract cache, compile core tests

51f84e3

Remove 2.11-specific config

8691199

lewisjkl reviewed Jun 1, 2020

View reviewed changes

paulpdaniels reviewed Jun 19, 2020

View reviewed changes

lewisjkl added 3 commits July 3, 2020 07:22

remove extra in RedisClusterCache#doPut

d772729

update docs to get them compiling

4656021

fix mdoc cleanup to close caches by running cache.close

3a991ba

lewisjkl reviewed Jul 3, 2020

View reviewed changes

Remove a couple of TODOs

d1c29ad

cb372 marked this pull request as ready for review July 31, 2020 15:35

cb372 merged commit 7fdf424 into cb372:master Jul 31, 2020

cb372 mentioned this pull request Jul 31, 2020

Remove dependencies on me #410

Open

4 tasks

kubukoz deleted the cats-effect branch July 31, 2020 16:23

kubukoz mentioned this pull request Dec 9, 2020

Introduce logging abstraction #455

Open

		@@ -3,6 +3,7 @@ import scala.language.higherKinds

		package object scalacache {

		//todo

Add cats-effect to the core module, remove modes #345

Add cats-effect to the core module, remove modes #345

Conversation

kubukoz commented Feb 7, 2020

lewisjkl commented Feb 7, 2020

cb372 commented Feb 7, 2020

kubukoz commented Feb 7, 2020

cb372 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewisjkl commented Feb 13, 2020

kubukoz commented Feb 13, 2020

lewisjkl commented Feb 17, 2020

cb372 commented Feb 25, 2020

kubukoz commented Mar 15, 2020

kubukoz commented Mar 22, 2020

lewisjkl commented Mar 23, 2020

kubukoz commented Mar 23, 2020

lewisjkl commented May 27, 2020

kubukoz commented May 27, 2020

cb372 commented May 28, 2020

kubukoz commented May 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewisjkl commented Jun 1, 2020

kubukoz commented Jun 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubukoz Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

cb372 commented Jun 27, 2020

lewisjkl commented Jun 27, 2020

lewisjkl commented Jun 29, 2020

cb372 commented Jul 2, 2020

lewisjkl commented Jul 2, 2020

lewisjkl commented Jul 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewisjkl commented Jul 3, 2020

cb372 commented Jul 31, 2020

kubukoz commented Jul 31, 2020

lewisjkl commented Jul 31, 2020

kubukoz Jun 19, 2020 •

edited

Loading